On Mon, 28 Sep 2015 21:06:58 -0400
Greg Reagle <greg.reagle_AT_umbc.edu> wrote:
Hey Greg,
> This is very limited at this point.
thank you very much for your contribution! I haven't had the chance
to run the code yet, but let me first give you some bits on the code
itself.
I'm impressed how well you already achieved the sbase-style. Very
good work, especially regarding the argv-loop-idiom and other things!
1) © 2014,2015 Greg Reagle <greg.reagle_AT_umbc.edu>
better use
© 2014-2015 Greg Reagle <greg.reagle_AT_umbc.edu>
2) writes an octal dump of
.Ar file(s)
to standard output. If no
.Ar file(s)
are specified then
.Nm
is a filter, i.e. reads from standard input.
Better use consistent wording with the other manpages:
writes an octal dump of each
.Ar file
to stdout. If no
.Ar file
is given,
.Nm
reads from stdin.
The intent is good to point out that it acts as a filter then,
but if we add it here, it somehow would effectively be lacking
everywhere else.
The core utilities consist of many filters.
3) Try to avoid blank lines in mandoc. Debug more issues with
mandoc -Tlint.
4) #include <string.h>
#include "stdlib.h"
#include "util.h"
There is an obvious error here. ;)
5) As I've not yet run the code, I'm a bit confused by the
deployment of usage() here. Is it possible usage is
printed after the program has given output?
If yes, rather try to work with exit codes and handle
those errors more or less centrally in main().
As soon as we have printed something to the opened files,
it's desirable to run fshut() on the given files as well
to flush the buffers and occasionally properly report
conditions when we run out of space (pipe to /dev/full
for testing).
6) size_t n; /*actual size of the buffer*/
size_t i; /*tracks index within the buffer*/
Try adding spaces after inside the comments (/* ... */).
Also, why not think of a better naming to convey the
meaning of these variables better inside the code?
If that is not possible, then the comments are fine.
7) Try ordering variable declarations by size, starting with
structs, then names types (e.g., Rune), and then by size,
starting with largest and going to smallest (size_t, uint,
int, char, ...).
Unsigned types could be considered larger, as they can
represent absolutely larger numbers.
8) while ((n = fread(buf, 1, 5, fp_in))) {
Just use for(; n = fread(buf, 1, 5, fp_in; )
9) if ((strlen(address_radix) > 1) || (!strchr("doxn", address_radix[0])))
Break up this long statement. Break it after the OR.
10) for (i=0; i<n; ++i, ++addr) {
Don't forget the spacing ;)
for (i = 0; i < n; ++i, ++addr) {
11) Feel free to also remove od from the TODO in your patch
and add it to the README (of course, without audit, but
in the process of POSIX compliance[0])
Again, thank you very much for your work! This is a pretty
important tool. :)
I'll also take my time and comment on your patch adding the
t-flag.
Cheers
FRIGN
[0]:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/od.html
--
FRIGN <dev_AT_frign.de>
Received on Tue Sep 29 2015 - 10:46:35 CEST