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