[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!