[Toybox] [CLEANUP]

[Rob Landley]

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