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

Commit Message

Junio C Hamano May 8, 2020, 3:18 p.m. UTC

Christian Couder <[e-mail]> writes:

> On Fri, May 8, 2020 at 7:51 AM Shourya Shukla
> <[e-mail]> wrote:
>>
>> On 06/05 10:08, Junio C Hamano wrote:
>
>> > Specifically, I would write "!path", not "path == NULL".  I thought
>> > a rule for that is in the CodingGuidelines (I didn't double check,
>> > though).
>>
>> I could not find a rule like that in the CodingGuidelines.
>> Should I add it?
>> https://github.com/git/git/blob/master/Documentation/CodingGuidelines
>
> Sure.

I'd rather not see too many unrelated things piled up on Shourya's
plate.  Without guidance, the new entry we'll see would be only
about comparing with NULL, and we'd need to spend review cycles
correcting that, too.

How about something like this, perhaps?

-- >8 --
CodingGuidelines: do not ==/!= compare with 0/NULL

Signed-off-by: Junio C Hamano <[e-mail]>
---
 Documentation/CodingGuidelines | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Eric Sunshine May 8, 2020, 3:38 p.m. UTC | #1

On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <[e-mail]> 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 '=='. 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.

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).

Junio C Hamano May 8, 2020, 3:57 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <[e-mail]> 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.

> 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?

> 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.

Eric Sunshine May 8, 2020, 4:13 p.m. UTC | #3

On Fri, May 08, 2020 at 08:57:09AM -0700, Junio C Hamano wrote:
> Eric Sunshine <[e-mail]> writes:
> > This talks only about '=='.
> 
> Yup.  The text would need a matching change, though.

Here's a re-roll with the necessary changes.

> >     if (!ptr)
> >         BUG("...");
> >     if (cnt)
> >         foo(ptr, cnt);
> 
> Or more succinctly:
> 
> 	if (!ptr || cnt)
> 		BUG("we must have an empty array at this point");

I considered that but thought it might be too "cute", however, seeing
it written out, it looks fine, so I used it in the re-roll.

> > Also, would you want to talk about not comparing against NUL character?
> 
> Yeah, it might be worth saying it explicitly.  I dunno.

Rather than giving this a separate example in the re-roll, I just
mentioned '\0' in the text.

--- >8 ---

From: Junio C Hamano <[e-mail]>
Subject: [PATCH] CodingGuidelines: do not ==/!= compare with 0 or '\0' or NULL

Signed-off-by: Junio C Hamano <[e-mail]>
Signed-off-by: Eric Sunshine <[e-mail]>
---
 Documentation/CodingGuidelines | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 390ceece52..6dfc47ed7d 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -236,6 +236,18 @@ For C programs:
         while( condition )
 		func (bar+1);
 
+ - Do not explicitly compare an integral value with constant 0 or '\0',
+   or a pointer value with constant NULL.  For instance, to validate a
+   counted array ptr that has cnt elements, write:
+
+	if (!ptr || cnt)
+		BUG("empty array expected");
+
+   and not:
+
+	if (ptr == NULL || cnt != 0);
+		BUG("empty array expected");
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {

Junio C Hamano May 8, 2020, 4:38 p.m. UTC | #4

Eric Sunshine <[e-mail]> writes:

> From: Junio C Hamano <[e-mail]>
> Subject: [PATCH] CodingGuidelines: do not ==/!= compare with 0 or '\0' or NULL
>
> Signed-off-by: Junio C Hamano <[e-mail]>
> Signed-off-by: Eric Sunshine <[e-mail]>
> ---
>  Documentation/CodingGuidelines | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 390ceece52..6dfc47ed7d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -236,6 +236,18 @@ For C programs:
>          while( condition )
>  		func (bar+1);
>  
> + - Do not explicitly compare an integral value with constant 0 or '\0',
> +   or a pointer value with constant NULL.  For instance, to validate a
> +   counted array ptr that has cnt elements, write:

I think this should be

      counted array <ptr, cnt> is initialized but has no elements, write:

> +
> +	if (!ptr || cnt)
> +		BUG("empty array expected");
> +
> +   and not:
> +
> +	if (ptr == NULL || cnt != 0);
> +		BUG("empty array expected");
> +
>   - We avoid using braces unnecessarily.  I.e.
>  
>  	if (bla) {

Eric Sunshine May 8, 2020, 5:51 p.m. UTC | #5

On Fri, May 08, 2020 at 09:38:17AM -0700, Junio C Hamano wrote:
> Eric Sunshine <[e-mail]> writes:
> > + - Do not explicitly compare an integral value with constant 0 or '\0',
> > +   or a pointer value with constant NULL.  For instance, to validate a
> > +   counted array ptr that has cnt elements, write:
> 
> I think this should be
> 
>       counted array <ptr, cnt> is initialized but has no elements, write:

You're right. Here's a corrected version. I also applied s/a/that/ in
the second line to improve the grammar a bit.

--- >8 ---

From: Junio C Hamano <[e-mail]>
Subject: [PATCH v3] CodingGuidelines: do not ==/!= compare with 0 or '\0' or
 NULL

Signed-off-by: Junio C Hamano <[e-mail]>
Signed-off-by: Eric Sunshine <[e-mail]>
---
 Documentation/CodingGuidelines | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 390ceece52..803a3b9bde 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -236,6 +236,18 @@ For C programs:
         while( condition )
 		func (bar+1);
 
+ - Do not explicitly compare an integral value with constant 0 or '\0',
+   or a pointer value with constant NULL.  For instance, to validate that
+   counted array <ptr, cnt> is initialized but has no elements, write:
+
+	if (!ptr || cnt)
+		BUG("empty array expected");
+
+   and not:
+
+	if (ptr == NULL || cnt != 0);
+		BUG("empty array expected");
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {

Patch

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 390ceece52..41a89dd845 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -236,6 +236,19 @@  For C programs:
         while( condition )
 		func (bar+1);
 
+ - 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");
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {