Re: Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2020, 15:57 UTC
Message-ID: <xmqqpnbezaga.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cQP_9onrq-z5db1GhXSSHaeKJ+UhNewWP25wLCsMRzSrA@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 15 quoted lines

> On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>> + - Do not explicitly compare an integral value with constant 0 or a
>> +   pointer value with constant NULL for equality; just say !value
>> +   instead.  To validate a counted array at ptr that has cnt elements
>> +   in it, write:
>> +
>> +       if (!ptr || !cnt)
>> +               BUG("array should not be empty at this point");
>> +
>> +   and not:
>> +
>> +       if (ptr == NULL || cnt == 0);
>> +               BUG("array should not be empty at this point");
>
> This talks only about '=='.

Yup.  The text would need a matching change, though.

Show 16 quoted lines

> People might still use 0 or NULL with
> '!='. I wonder if the example can include '!=', as well. Perhaps:
>
>     if (!ptr)
>         BUG("...");
>     if (cnt)
>         foo(ptr, cnt);
>
> instead of:
>
>     if (ptr == NULL)
>         BUG("...");
>     if (cnt != 0)
>         foo(ptr, cnt);
>
> or something.

Or more succinctly:

	if (!ptr || cnt)
		BUG("we must have an empty array at this point");

perhaps?

Show 13 quoted lines

> Also, would you want to talk about not comparing against NUL character?
>
>     if (*s)
>         foo(s);
>
> instead of:
>
>     if (*s != '\0')
>         foo(s);
>
> Maybe that's overkill since NUL is an integral value which is already
> covered by your earlier statement (but perhaps some people would
> overlook that).

Yeah, it might be worth saying it explicitly.  I dunno.