sed-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PATCH: use gnulib modules instead of custom code


From: Jim Meyering
Subject: Re: PATCH: use gnulib modules instead of custom code
Date: Thu, 2 Aug 2018 18:09:39 +0200

On Wed, Aug 1, 2018 at 4:52 PM, Jim Meyering <address@hidden> wrote:
> On Tue, Jul 31, 2018, 08:56 Assaf Gordon <address@hidden> wrote:
>>
>> Hello,
>>
>> Several functions in the sed codebase have equivalent modules in gnulib
>> (likely did not exist when the code was originally written).
>>
>> These 5 patches replace them:
>>
>> sed: replace myname with gnulib's progname
>> sed: replace ck_realloc with gnulib's xrealloc/xnrealloc
>> sed: replace MALLOC/ck_malloc with gnulib's XCALLOC
>> sed: replace ck_strdup with gnulib's xstrdup
>> sed: replcae ck_memdup with gnulib's xmemdup
>
> Thanks a lot for doing all that. Those have irritated me for some time.
> Can't review at this moment, but will tomorrow.

I've started, and found no issue with the first one. Thanks! Please push that.

In the second, I note that the removed ck_realloc function would
sometimes zero-fill an allocated buffer, yet the replacement function,
xnrealloc, never does. However, I suspect that no caller relies on
that now-removed zero-filling, and I think the code is better without
it -- might even make a noticeable performance improvement. If some
caller requires it, we can add that zero-filling at a higher level.
Did you test with valgrind and/or ASAN? In other words, I like your
second patch, too, assuming all tests pass when using
ASAN/UBSAN-enabled binaries.

The fourth, switching to xstrdup looks fine.
In the title of the 5th patch, s/replcae/replace/

As for the third, did you consider not zero-filling any of those
buffers? IMHO, that can often be avoided at minimal/no cost to
readability and maintainability. However, I'm happy to accept these
changes and leave the more invasive and potentially riskier
optimizations to a later time.

Bottom line: with that one typo fix and given my assumption on #2, I'd
be happy if you were to push all five.
Thanks again!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]