I blogged a bit (http://landley.net/notes-2013.html#31-03-2013) about
how I'd like more cleanup submissions. Possibly if I explain the kind
of cleanup I'm going for, it might help with that.
When ifconfig was submitted, it touched a half-dozen files. I glued it
together into a single self-contained file, which needs a lot of
cleanup.
The reason I collected the half-dozen files is that nothing else is
using any of that code yet. I try to avoid what I call "infrastructure
in search of a user". Putting code into common directories should wait
until it has more than one command actually calling it.
Another thing I'm trying to do is collect code together so functions
and their callers are in physical proximity, because that lets you
inline, untangle, and simplify. For example, code that does:
#define CONSTANT 42
printf("%d", CONSTANT);
There's no reason for the #define, you can just substitute it straight
in the only place it's ever used. But if define is far away, you can't
always see that it's not needed.
Defines are actually a specific category of bloat, because people like
to pull values out and collect them together in what they think of as
configuration sections. But this isn't always an improvement. Asking
whether this config knob will ever be changed, and even if it is would
it be any harder to find it inline in one or two places that actually
use it than figuring out you need to change the config block? (Which
involves reading the code and understanding what it's doing anyway to
know that changing the #define is enough to change the behavior, and
there's not some other hardwired assumption that the change breaks?)
Let's look at an example:
http://landley.net/hg/toybox/rev/843
Commit 843 was a recent cleanup to ifconfig. The first hunk I changed
is:
--- a/toys/pending/ifconfig.c Thu Apr 04 19:39:44 2013 -0500
+++ b/toys/pending/ifconfig.c Thu Apr 04 20:27:08 2013 -0500
@@ -37,14 +37,6 @@
#include <net/ethernet.h>
#include <alloca.h>
-#define TRUE ((int) 1)
-#define HTTP_DEFAULT_PORT 80
-#define FTP_DEFAULT_PORT 21
-
-#ifndef MAX_PORT_VALUE
-#define MAX_PORT_VALUE 65535
-#endif
-
Let's look at that. TRUE is 1. This is built into C. (c99 actually says
that any nonzero value is true in a comparison function like if (), but
that logical operators && and || and == will return 0 or 1. So you can
pull tricks like rewriting:
if (y == 4) x = 20;
else x = 7;
mangle(thingy, x);
as
mangle (thingy, 7+(y==4)*13);
And it's actually portable on a c99 compiler. Whether or not it's
better code is complicated (digression warning: this parenthetical is
irrelevant. Modern processors rely heavily on speculative execution to
keep multiple cores busy and work around prefetch stalls and other
pipeline bubbles, and avoiding branches makes speculative execution
much more efficient because if it guesses the wrong branch it wastes
work it has to discard. But in this case a modern processor probably
has conditional assignment instructions, although more power efficient
architectures do less speculative execution because wasted work still
comsumes power so avoiding it slows absolute speed but increases
battery efficiency. But if you're plugged into wall current you can
statistically turn some of those wasted cycles into performance by
being lucky guessing what comes next.)
The reason _I_ like the one line version instead of the three line
version is it's a lot less code to read. If you fit more on the screen
it's easier to keep what it's doing in your head, because it's right
there in front of you instead of something you have to flip back and
forth to follow.
Going back to the #define TRUE: in theory there's probaby some header
we could #include to get this, but there turned out to be exactly one
user, resulting in this hunk to remove it:
@@ -1483,7 +1419,7 @@
perror_msg("error: no inet socket available");
return -1;
}
- while(TRUE) {
+ for (;;) {
ifcon.ifc_len = sizeof(struct ifreq) * num_of_req;
//Size of buffer.
ifcon.ifc_buf = xrealloc(ifcon.ifc_buf, ifcon.ifc_len);
Saying while (1) wouldn't be any less clear than while (TRUE) if you're
at all familiar with C, and saying for (;;) lets you not have a test at
all. (The compiler edits out constant true/false tests, but still. I
find it cleaner to not have them if they're not doing anything.)
Going back to the rest of that #define block: HTTP_DEFAULT_PORT and
FTP_DEFAULT_PORT are 20 year old standardized values. This isn't a
config knob anybody can ever change. same for MAX_PORT_VALUE, it's
hardwired for both IPv4 and IPv6, and changing this code to work with
other packet procols is pretty much a complete rewrite anyway.
The two DEFAULT_PORT macros are each only used once, in a function that
is never called from anywhere, nor is the one after it. Easy cleanup
that: remove the lot of it. (It's easy enough to put back if something
does start using it. And if so, just use the constants. If they really
need a comment to explain that 80 is HTTP, add a comment, but if you're
doing network stuff that may be common knowledge by now and what the
function was doing was strcmp(blah, "http") and strcmp(blah, "ftp") so
it's there in the code without needing a comment anyway. So if we
hadn't removed the function, the defines still would have gone away.)
MAX_PORT_VALUE was used twice, both times to sanity check a command
line input. There's range checking built into the lib/args.c option
parsing stuff, you can go "#<1>65535". I dunno if it applies here, but
a todo item would be to check that. Also, why is ifconfig dealing with
port numbers? For now just swap in the constants and make the #define
go away.
And that's one cleanup pass.
Rob
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
