Re: [hackers] created od

From: FRIGN <dev_AT_frign.de>
Date: Tue, 29 Sep 2015 10:46:35 +0200

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

This archive was generated by hypermail 2.3.0 : Tue Sep 29 2015 - 10:48:11 CEST