Review of the last few applied patches...

Bernhard Fischer rep.nop at aon.at
Fri May 19 18:14:46 UTC 2006


On Fri, May 19, 2006 at 01:28:05PM -0400, Rob Landley wrote:
>Review of yesterday's committed patches...
>
>15134: fine.
>
>15133:
>Since /dev/null isn't going to move, this is just a size optimization, and
>my question is does building all sources at once get this for us?  If so,
>we don't need the complexity, and should probably remove bb_dev_null.  (It'll
>be 2-3 years before building all sources at once becomes the recommended way
>to do it, but we should start thinking about it now and avoid some cleanups
>we'd just have to revert.)
>
>We should check and make sure that building all sources at once _does_ merge
>duplicate strings.  Let's see...  make clean, make defconfig, switch on
>build at once, then "strings busybox | sort | uniq -d"...
>
>Oh dear.  Somebody needs to poke the gcc guys.  We've got _lots_ of
>duplicate strings that aren't being merged.  Apparently, we've even got
>two copies of the CONFIG ones (for example, CONFIG_ADDGROUP is on line 5474
>and line 7608 of the "strings busybox" output for the build at once version
>configured with defconfig).  Why do we have two copies of the CONFIG strings
>in the busybox binary?
>
>I just hand edited the duplicate strings output to remove the false positives,
>and there's 615 left.  (See attachment.)

do you mean --merge-all-constants ?. Sounds a bit dangerous and
doesn't seem to gain much:
$ size busybox_old busybox_unstripped 
   text	   data	    bss	    dec	    hex	filename
 848951	   9100	 645216	1503267	 16f023	busybox_old
 848735	   9100	 645216	1503051	 16ef4b	busybox_unstripped

I think we should try to (manually) reuse as much strings as possible
anyway.
>
>15132: fine (see 15133)
>
>15131: mostly fine, although I believe you don't need USE() around a local
>variable declaration unless the code that uses it is entirely #ifdefed out and
>you want to avoid the warning.  If the code using a variable is dead code
>eliminated, then the variables should be too, without a warning.
>
>Although again, my understanding of what a sane optimizer would do may not
>match the reality of gcc...
>
>15129: fine (I'd have tried to clean up the #ifdef while I was there, but
>that's a separate issue.)
>
>15128: I'd have removed it.

done.
>
>15126: Fine.  Should add the llist note to the TODO.  In fact I should go
>through the todo thread from last month and add all the minor todo items
>to the TODO.

Please do.
>
>15124: What is strings.h needed for again?  Does this #include make a warning

str{,n}casecmp()

>go away, or is some future "remove unnecessary #includes" pass going to just
>undo this again?  There's no rationale as to _why_ this was included.  Maybe
>it was in the email.

It was in the checkin message for the -stable branch.
I'm not going to start to whine about the maintainer not maintaining the
stable codebase in a collaborative manner.. will not.. will... not
>
>15122: ok.
>
>15121: I was trying to poke Robert Day into getting and using an SVN 
>account. :P
>
>15119: Nitpick: If you're going to fix the comment why didn't you fix the
>\ after the ` to be an /?  The code itself looks fine...

I admit that i overread the wrong tick in Yann's patch.



More information about the busybox mailing list