On Wednesday 09 January 2008 11:11:04 Natanael Copa wrote:
>
> On Tue, 2008-01-08 at 22:42 +0100, Tito wrote:
> > Hi,
> > this new version of tac.c handles embedded nulls in a correct way.
> > I tested it in comparison with the real tac on most of the files
> > of my hard disk with the attached script and it seems to work.
>
> Good work! It's nice to work with you :)
:-)
>
> > Before posting a patch I would like the list members to take
> > a look at this new approach for hints, improvements and critics.
> > For the same reason I have not checked the size increase yet.
>
> Is it not horribly slow to call xrealloc for every single char? Its
> probably not a big deal for normal tac use (small files).
>
> Should xrealloc really be used? (or should the comment about NOEXEC be
> removed). I guess something like this is more appropiate if we want to
> keep it as a NOEXEC app:
> line = realloc(...);
> if (line == NULL)
> goto clean_up_and_exit_with_err;
>
Previous version marked as noexec used xmalloc_fgets(f),
so I thought it is ok to use xrealloc directly.
>
> I dont know if it makes any difference in size but:
> if (ch != EOF)
> line[i++] = ch;
> if (ch == '\n' || ch == EOF) {
> /* Save the size of the line to the list */
> ...
>
> is more readable than:
> if (ch == EOF)
> goto line_end;
> line[i] = ch;
> i++;
> if (ch == '\n' || ch == EOF) {
> line_end:
> /* Save the size of the line to the list */
> ...
Haven't checked the size but your version looks cleaner.
>
> (besides, the ' || ch == EOF' will never be evaulated since if ch is
> EOF, then had the goto above already jumped over this part)
>
Was a left-over of a previous version and could be removed.
> You test script didnt work so well for me. I attatched an improved one
> (use 'cmp' rather than 'diff')
Yours work for me :-)
>
> I have added a patch (against svn).
>
> Bloatcheck on x86_64 compared to current svn:
> function old new delta
> tac_main 227 294 +67
> xmalloc_fgets 19 - -19
> bb_get_chunk_from_file 153 - -153
> ------------------------------------------------------------------------------
> (add/remove: 0/2 grow/shrink: 1/0 up/down: 67/-172) Total: -105
> bytes
> text data bss dec hex filename
> 68517 1374 256 70147 11203 busybox_old
> 68313 1374 256 69943 11137 busybox_unstripped
>
>
> Bloatcheck on x86_64 compared to Tito's latest:
> function old new delta
> tac_main 333 294 -39
> ------------------------------------------------------------------------------
> (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-39) Total: -39
> bytes
> text data bss dec hex filename
> 68352 1374 256 69982 1115e busybox_old
> 68313 1374 256 69943 11137 busybox_unstripped
>
> Size reduction here is mostly due to use of a struct with both size and
> buf, lstring.
Yes this is a cleaner solution.
So let's see what Denis thinks about it.
Ciao,
Tito
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
