Review of the last few applied patches...

[Rob Landley]

On Friday 19 May 2006 2:14 pm, Bernhard Fischer wrote:
> >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.

A) If it really is constant it doesn't _sound_ dangerous.
B) The file I attached was 13k, which is noticeably larger than 200 bytes. :)

> >15128: I'd have removed it.
>
> done.

Woot.

> >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.

I have a todo item to collate the todo lists.  This is _sad_.

> >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.

Under linux, #include <string.h> defines strcasecmp(), which is why we didn't 
see a warning.  You don't need strings.h.  In fact having a separate 
"string.h" and "strings.h" at all strikes me as deep and abiding stupidity on 
the part of somebody upstream.  There's no _CHANCE_ to keep those straight in 
one's head, and no obvious reason to try.  (Hence linux including one from 
the other for sanity.)

What exactly is the problem being solved here?  (Doesn't build on some 
platform that's more pedanitc/less sane than Linux?  Which one?)

> >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.

I said it was a nitpick...

Rob
-- 
Never bet against the cheap plastic solution.