On 04/16/2013 10:18:59 AM, Kyungwan Han wrote:
Hello,
I learn programming skill and knowledge from following up code
cleaning,
so I'm checking the change set:
http://www.landley.net/hg/toybox/rev/6be04ec7b7ac
Ah, I never did summarize that one, did I? Sorry, my day job at Cray's
been taking up all my time...
Ok, that's commit 852:
http://landley.net/hg/toybox/rev/852
(Note: every mercurial commit has a sequentially increasing serial
number, which is just the order the commits went in to that instance of
the repository. Since my repository's the master copy I can cheat
slightly and just use the index numbers when referring to commits in
the repository on landley.net. I find 'em easier to deal with than the
big commit hash.)
Ok, I switched the config default from "y" to "n", because stuff in
pending shouldn't be enabled by default. (Minor administrative thing,
it'll go back to "y" when it's ready to move out of pending.)
I removed the comments about "function declaration" and "structure
declaration" because it's documenting the obvious. (It's not like
there's hundreds of entries where the comment gives us an easy way to
find the start of a large section via text search.)
In the case of function declarations, if the functions are in the right
order we don't need to declare them ahead of time. Declarations are
really so functions can be used in a different file, it mostly
shouldn't be needed in the same file unless you have circular calls.
Also, when we inline code functions go away along with the need to
declare them before use. So declarations local to a single file are a
rough edge I try to clean up. (Doesn't mean it's always possible, but
it attracts my attention.)
Yes, I removed "const". Isaac and Felix linked you to the earlier post
on that. It's not an absolute prohibition, but in this case it shrinks
the code without really losing anything.
Functions returning void (like get_host_and_port()) don't need a return
statment on the last line, it doesn't do anything.
get_socket_stream():
Collate local variable declarations of the same type. (Sometimes it's
worthwhile keeping structure members on separate lines to make it clear
what order the structure members are in, especially when it's
significant as in the args.c parsing of GLOBALS(), which is really an
implicit union between a long array and the start of the structure. But
in local variables, order isn't significant.)
The perror_exit() function already appends a colon and strerror() to
the end of its output. (error_exit() doesn't, perror_exit() does.) so
trim off the code that does that here. Also, both error_exit() and
perror_exit() append a newline to the string they print, so if the
error strings we pass them do that the newline gets printed twice.
Remove a few unnecessary parentheses, if (a == b || c == d) is obvious
enough.
At the end of get_socket_stream(), status has to be 0 or we wouldn't
have gotten there, and swap the ? : so we don't have to !rp (removing
more parentheses).
get_sockaddr: Remove is_unix and status variables that are only used to
store a value used exactly once on the next line, just inline the call
generating the value.
Simplify setport(), and then inline it at the only call site. (Looks
like I forgot to actually _delete_ the setport function and its
declaration once it was no longer used, I'll do that in my pending next
round of cleanup.)
Moving on to get_strtou():
Switching argv[0] to *argv only saves a couple characters but treating
a pointer as an array instead of simply dereferncing it only really
makes sense when you use more than one element of the array. (If you
have a pointer to struct and go ptr[0].blah instead of ptr->blah it
implies you're not really comfortable with pointers.)
Comments like "end of outfill" don't really help when the if() is on
the screen at the same time. (And if it isn't, can the body of the if
be shrunk enough that it is? Those are generally for really _big_
blocks, and even then your editor will usually jump you between matched
curly brackets.)
Note the char * typecast removed in set_irq() was already a char *, the
cast was there to stop the compiler complaining that the char * was
const. This is part of the reason I don't like const annotations, it
doesn't _stop_ you from modifying it, it just makes the compiler
complain. I've done enough python programming to not hugely care what
the compiler has to say at compile time, I want good _runtime_ tests of
the behavior before I'll trust it. A test suite executing all the
corner cases and maybe some valgrind variant spotting accumulating
memory leaks and such beats "lint" any day.
get_hw_info(): remove a #define that only exists for the lifetime of
the function. Just substitue the value in the one place it's used.
(Also, the #define HW_UNSPEC _and_ the comment // UNSPEC on its only
use are one line before we strcpy "unspec" in response to it, which is
self-documenting.)
I also moved the memset() from right before the only caller of
get_hw_info() into the function itself. In a future pass I'll inline it
at the only call site, and then see what simplification opportunities
that affords us.
Replace xprintf("\n"); with xputc('\n') because the string is a two
byte global constant (the character pluss a null) byte in a separate
block of memory with a pointer to it (and associated ELF plumbing), and
what it's loading onto the stack is a pointer to that global. With
xputc we're just loading an 8 bit constant (promoted to 32 bits by C,
but still smaller than a pointer and there's nothing to dereference).
Not a huge deal, but a general case of "xputc exists for a reason,
might as well use it".)
readconf(): remove another function-local #define/#undef that's used
exactly once, just inline the constant. Putting the constant in a
#define doesn't get rid of the constant, it just adds indirection and
makes the code bigger.
Rob
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
