Re: parse-options: avoid arithmetic on pointer that's potentially NULL
- From
René Scharfe <l.s.r@web.de>
- Date
- Nov 13, 2019, 16:48 UTC
- Message-ID
- <df941c25-22ea-3523-9607-8e892dea320c@web.de>
- In-Reply-To
- <xmqq5zjocx7i.fsf@gitster-ct.c.googlers.com>
Am 13.11.19 um 03:43 schrieb Junio C Hamano:
Show 12 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>
>> The function is also prepared to handle a NULL pointer passed to it.
>> None of its callers do that currently, but this feature was used by
>> 46e91b663b ("checkout: split part of it to new command 'restore'",
>> 2019-04-25); it seems worth keeping.
>
> ... meaning it was used but since then we rewrote the user and there
> is no caller that uses it?
>
> Gee, how are you finding such an instance of use? I am quite
> impressed.I wondered why the function checks for NULL, noticed that both users are in builtin/checkout.c and pass a pointer to a struct, so I ran "git log -p builtin/checkout.c" and searched for "= NULL" in its output because I suspected or vaguely remembered that the options were added one by one by a patch series. Sloppy, but good enough in this case..
Show 18 quoted lines
>> struct option *parse_options_dup(const struct option *o)
>> {
>> + const struct option *orig = o;
>
> If I were doing this patch, I'd give the incoming parameter a more
> meaningful name and the temporary/local variable that is used to
> scan would get/keep the shorter name. IOW
>
> struct parse_options_dup(const struct option *original)
> {
> const struct option *o = original;
> struct option *copied;
> int nr = 0;
>
> ... loop to count ...
> ALLOC_ARRAY(copied, nr + 1);
> COPY_ARRAY(copied, original, nr);
> ...Right. And I just rediscovered an unsent patch series from last summer that refactors this function further. It fixed the NULL issue as well, but only by accident. Will rethink/reintegrate these patches and send them later, but here's one that's ready and fits the COPY_ARRAY theme:
-- >8 -- Subject: [PATCH] parse-options: use COPY_ARRAY in parse_options_concat()
Use COPY_ARRAY to copy whole arrays instead of iterating through their elements. That's shorter, simpler and bit more efficient.
Signed-off-by: René Scharfe <l.s.r@web.de> --- parse-options-cb.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c index c2062ae742..8b443938b8 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -188,10 +188,8 @@ struct option *parse_options_concat(struct option *a, struct option *b) b_len++; ALLOC_ARRAY(ret, st_add3(a_len, b_len, 1)); - for (i = 0; i < a_len; i++) - ret[i] = a[i]; - for (i = 0; i < b_len; i++) - ret[a_len + i] = b[i]; + COPY_ARRAY(ret, a, a_len); + COPY_ARRAY(ret + a_len, b, b_len); ret[a_len + b_len] = b[b_len]; /* final OPTION_END */ return ret; -- 2.24.0