git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
@ 2019-07-11 22:03 Ramsay Jones
  2019-07-12  5:21 ` Johannes Sixt
  0 siblings, 1 reply; 21+ messages in thread
From: Ramsay Jones @ 2019-07-11 22:03 UTC (permalink / raw)
  To: Thomas Gummerer; +Cc: Junio C Hamano, GIT Mailing-list


Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Thomas,

If you need to re-roll your 'tg/range-diff-output-update' branch, could
you please squash (parts) of this into the relevant patches.

The first hunk fixes a couple of 'hdr-check' warnings:

  $ diff nhcout phcout | head
  4a5,13
  > apply.h:146:22: error: ‘GIT_MAX_HEXSZ’ undeclared here (not in a function); did you mean ‘NI_MAXHOST’?
  >   char old_oid_prefix[GIT_MAX_HEXSZ + 1];
  >                       ^~~~~~~~~~~~~
  >                       NI_MAXHOST
  > apply.h:151:19: error: array type has incomplete element type ‘struct object_id’
  >   struct object_id threeway_stage[3];
  >                    ^~~~~~~~~~~~~~
  > Makefile:2775: recipe for target 'apply.hco' failed
  > make: *** [apply.hco] Error 1
  $ 

and needs to be applied to commit b9f62a7e24 ("apply: make parse_git_header
public", 2019-07-08).

The second hunk fixes a sparse 'Using plain integer as NULL pointer'
warning, and needs to be applied to commit 04539fc67b ("range-diff: add
section header instead of diff header", 2019-07-08).

Thanks!

ATB,
Ramsay Jones

 apply.h      | 1 +
 range-diff.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/apply.h b/apply.h
index ade50f66c5..c8c9287cb2 100644
--- a/apply.h
+++ b/apply.h
@@ -3,6 +3,7 @@
 
 #include "lockfile.h"
 #include "string-list.h"
+#include "hash.h"
 
 struct repository;
 
diff --git a/range-diff.c b/range-diff.c
index ba1e9a4265..0f24a4ad12 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -102,7 +102,7 @@ static int read_patches(const char *range, struct string_list *list)
 		}
 
 		if (starts_with(line, "diff --git")) {
-			struct patch patch = { 0 };
+			struct patch patch = { NULL };
 			struct strbuf root = STRBUF_INIT;
 			int linenr = 0;
 
-- 
2.22.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-11 22:03 [PATCH] range-diff: fix some 'hdr-check' and sparse warnings Ramsay Jones
@ 2019-07-12  5:21 ` Johannes Sixt
  2019-07-12 16:44   ` Junio C Hamano
  0 siblings, 1 reply; 21+ messages in thread
From: Johannes Sixt @ 2019-07-12  5:21 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Thomas Gummerer, Junio C Hamano, GIT Mailing-list

Am 12.07.19 um 00:03 schrieb Ramsay Jones:
> diff --git a/range-diff.c b/range-diff.c
> index ba1e9a4265..0f24a4ad12 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -102,7 +102,7 @@ static int read_patches(const char *range, struct string_list *list)
>  		}
>  
>  		if (starts_with(line, "diff --git")) {
> -			struct patch patch = { 0 };
> +			struct patch patch = { NULL };

There is nothing wrong with 0 here. IMHO, zero-initialization should
*always* be written as = { 0 } and nothing else. Changing 0 to NULL to
pacify sparse encourages a wrong style.

-- Hannes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-12  5:21 ` Johannes Sixt
@ 2019-07-12 16:44   ` Junio C Hamano
  2019-07-13 10:44     ` Johannes Sixt
  0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2019-07-12 16:44 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Ramsay Jones, Thomas Gummerer, GIT Mailing-list

Johannes Sixt <j6t@kdbg.org> writes:

> Am 12.07.19 um 00:03 schrieb Ramsay Jones:
>> diff --git a/range-diff.c b/range-diff.c
>> index ba1e9a4265..0f24a4ad12 100644
>> --- a/range-diff.c
>> +++ b/range-diff.c
>> @@ -102,7 +102,7 @@ static int read_patches(const char *range, struct string_list *list)
>>  		}
>>  
>>  		if (starts_with(line, "diff --git")) {
>> -			struct patch patch = { 0 };
>> +			struct patch patch = { NULL };
>
> There is nothing wrong with 0 here. IMHO, zero-initialization should
> *always* be written as = { 0 } and nothing else. Changing 0 to NULL to
> pacify sparse encourages a wrong style.
>
> -- Hannes

Hmm, care to elaborate a bit?  Certainly, we have a clear preference
between these two:

	struct patch patch;
	patch.new_name = 0;
	patch.new_name = NULL;

where the "char *new_name" field is the first one in the structure.
We always try to write the latter, even though we know they ought to
be equivalent to the language lawyers.

Is the reason why you say 0 is fine here because we consider

	struct patch patch, *dpatch;
	memset(&patch, 0, sizeof(patch));
	dpatch = xcalloc(1, sizeof(patch));

are perfectly good way to trivially iniitialize an instance of the
struct?

Do we want to talk to sparse folks about this?


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-12 16:44   ` Junio C Hamano
@ 2019-07-13 10:44     ` Johannes Sixt
  2019-07-13 12:18       ` Johannes Sixt
                         ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Johannes Sixt @ 2019-07-13 10:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramsay Jones, Thomas Gummerer, GIT Mailing-list

Am 12.07.19 um 18:44 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 12.07.19 um 00:03 schrieb Ramsay Jones:
>>> diff --git a/range-diff.c b/range-diff.c
>>> index ba1e9a4265..0f24a4ad12 100644
>>> --- a/range-diff.c
>>> +++ b/range-diff.c
>>> @@ -102,7 +102,7 @@ static int read_patches(const char *range, struct string_list *list)
>>>  		}
>>>  
>>>  		if (starts_with(line, "diff --git")) {
>>> -			struct patch patch = { 0 };
>>> +			struct patch patch = { NULL };
>>
>> There is nothing wrong with 0 here. IMHO, zero-initialization should
>> *always* be written as = { 0 } and nothing else. Changing 0 to NULL to
>> pacify sparse encourages a wrong style.
> 
> Hmm, care to elaborate a bit?  Certainly, we have a clear preference
> between these two:
> 
> 	struct patch patch;
> 	patch.new_name = 0;
> 	patch.new_name = NULL;
> 
> where the "char *new_name" field is the first one in the structure.
> We always try to write the latter, even though we know they ought to
> be equivalent to the language lawyers.

I'm not questioning this case; the latter form is clearly preferable.

Using only = { 0 } for zero-initialization makes the code immune to
rearrangements of the struct members. That is not the case with = { NULL
} because it requires that the first member is a pointer; if
rearrangement makes the first member a non-pointer, the initializations
must be adjusted.

On the other hand, I'm not arguing that

  struct string_list dup_it = { NULL, 0, 0, 1, NULL };

should be written as

  struct string_list dup_it = { 0, 0, 0, 1, 0 };

I'm only complaining about the single-initializer = { 0 } "please
initialize this whole struct with zero values" form.

> Is the reason why you say 0 is fine here because we consider
> 
> 	struct patch patch, *dpatch;
> 	memset(&patch, 0, sizeof(patch));
> 	dpatch = xcalloc(1, sizeof(patch));
> 
> are perfectly good way to trivially iniitialize an instance of the
> struct?

Absolutely not. Both forms are evildoing as far as struct initialization
is concerned because they ignore the types of the members. The memset
form should always be replaced by = { 0 }. The correct replacement for
the xcalloc form would be

	struct patch zero = { 0 };
	struct patch *dpatch = xmalloc(sizeof(*dpatch));
	*dpatch = zero;

but I do understand that this transformation is unacceptably verbose.

> Do we want to talk to sparse folks about this?

I've no idea which camp they are in. How would they respond to an
exceptional case that is also very much a matter of taste?

-- Hannes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 10:44     ` Johannes Sixt
@ 2019-07-13 12:18       ` Johannes Sixt
  2019-07-13 12:56       ` Carlo Arenas
  2019-07-13 21:29       ` Junio C Hamano
  2 siblings, 0 replies; 21+ messages in thread
From: Johannes Sixt @ 2019-07-13 12:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramsay Jones, Thomas Gummerer, GIT Mailing-list

Am 13.07.19 um 12:44 schrieb Johannes Sixt:
> Am 12.07.19 um 18:44 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Am 12.07.19 um 00:03 schrieb Ramsay Jones:
>>>> diff --git a/range-diff.c b/range-diff.c
>>>> index ba1e9a4265..0f24a4ad12 100644
>>>> --- a/range-diff.c
>>>> +++ b/range-diff.c
>>>> @@ -102,7 +102,7 @@ static int read_patches(const char *range, struct string_list *list)
>>>>  		}
>>>>  
>>>>  		if (starts_with(line, "diff --git")) {
>>>> -			struct patch patch = { 0 };
>>>> +			struct patch patch = { NULL };
>>>
>>> There is nothing wrong with 0 here. IMHO, zero-initialization should
>>> *always* be written as = { 0 } and nothing else. Changing 0 to NULL to
>>> pacify sparse encourages a wrong style.
>>
>> Hmm, care to elaborate a bit?  Certainly, we have a clear preference
>> between these two:
>>
>> 	struct patch patch;
>> 	patch.new_name = 0;
>> 	patch.new_name = NULL;
>>
>> where the "char *new_name" field is the first one in the structure.
>> We always try to write the latter, even though we know they ought to
>> be equivalent to the language lawyers.
> 
> I'm not questioning this case; the latter form is clearly preferable.
> 
> Using only = { 0 } for zero-initialization makes the code immune to
> rearrangements of the struct members. That is not the case with = { NULL
> } because it requires that the first member is a pointer; if
> rearrangement makes the first member a non-pointer, the initializations
> must be adjusted.
> 
> On the other hand, I'm not arguing that
> 
>   struct string_list dup_it = { NULL, 0, 0, 1, NULL };
> 
> should be written as
> 
>   struct string_list dup_it = { 0, 0, 0, 1, 0 };
> 
> I'm only complaining about the single-initializer = { 0 } "please

Of course, I'm not "complaining about" it; I'm "arguing for" it...

> initialize this whole struct with zero values" form.
> 
>> Is the reason why you say 0 is fine here because we consider
>>
>> 	struct patch patch, *dpatch;
>> 	memset(&patch, 0, sizeof(patch));
>> 	dpatch = xcalloc(1, sizeof(patch));
>>
>> are perfectly good way to trivially iniitialize an instance of the
>> struct?
> 
> Absolutely not. Both forms are evildoing as far as struct initialization
> is concerned because they ignore the types of the members. The memset
> form should always be replaced by = { 0 }. The correct replacement for
> the xcalloc form would be
> 
> 	struct patch zero = { 0 };
> 	struct patch *dpatch = xmalloc(sizeof(*dpatch));
> 	*dpatch = zero;
> 
> but I do understand that this transformation is unacceptably verbose.
> 
>> Do we want to talk to sparse folks about this?
> 
> I've no idea which camp they are in. How would they respond to an
> exceptional case that is also very much a matter of taste?
> 
> -- Hannes
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 10:44     ` Johannes Sixt
  2019-07-13 12:18       ` Johannes Sixt
@ 2019-07-13 12:56       ` Carlo Arenas
  2019-07-13 21:29       ` Junio C Hamano
  2 siblings, 0 replies; 21+ messages in thread
From: Carlo Arenas @ 2019-07-13 12:56 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Junio C Hamano, Ramsay Jones, Thomas Gummerer, GIT Mailing-list

On Sat, Jul 13, 2019 at 3:46 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Using only = { 0 } for zero-initialization makes the code immune to
> rearrangements of the struct members.

But only by forcing to set whichever is the first element to 0, which is
exactly what sparse is complaining about, since it happens to be a
pointer.

> That is not the case with = { NULL
> } because it requires that the first member is a pointer; if
> rearrangement makes the first member a non-pointer, the initializations
> must be adjusted.

luckily sparse will complain loudly in that case as well as shown by:

$ cat t.c
#include <stddef.h>

int main(int argc, char *argv[]) {
  struct {
    char i;
    char *p;
  } s = {NULL};
  return 0;
}
$ sparse t.c
t.c:7:10: warning: incorrect type in initializer (different base types)
t.c:7:10:    expected char i
t.c:7:10:    got void *

Carlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 10:44     ` Johannes Sixt
  2019-07-13 12:18       ` Johannes Sixt
  2019-07-13 12:56       ` Carlo Arenas
@ 2019-07-13 21:29       ` Junio C Hamano
  2019-07-13 22:22         ` Carlo Arenas
  2019-07-14  8:15         ` Johannes Sixt
  2 siblings, 2 replies; 21+ messages in thread
From: Junio C Hamano @ 2019-07-13 21:29 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Ramsay Jones, Thomas Gummerer, GIT Mailing-list

Johannes Sixt <j6t@kdbg.org> writes:

>> Hmm, care to elaborate a bit?  Certainly, we have a clear preference
>> between these two:
>> 
>> 	struct patch patch;
>> 	patch.new_name = 0;
>> 	patch.new_name = NULL;
>> 
>> where the "char *new_name" field is the first one in the structure.
>> We always try to write the latter, even though we know they ought to
>> be equivalent to the language lawyers.
>
> I'm not questioning this case; the latter form is clearly preferable.
>
> Using only = { 0 } for zero-initialization makes the code immune to
> rearrangements of the struct members. That is not the case with = { NULL
> } because it requires that the first member is a pointer; if
> rearrangement makes the first member a non-pointer, the initializations
> must be adjusted.

I do not think this position is maintainable, especially if you
agree with me (and everybody else, including sparse) that this is a
bad idea:

>   struct string_list dup_it = { 0, 0, 0, 1, 0 };

The way I read "6.7.8 Initialization" (sorry, I only have committee
draft wg14/n1124 of 2005 handy) is that

	struct patch patch = { 0 };

has an initializer for a structure with an automatic storage
duration, and for each of the subsequent fields of the structure
(i.e. we ran out the initializer after dealing with that single zero
that talks about the first field), due to "all subobjects that are
not initialized explicitly shall be initialized implicitly the same
as objects that have static storage duration." rule, "if it has a
pointer type, it is initialized to a null pointer", which is exactly
in line with your (and our) position that the first example I left
in the above (new_name gets assigned NULL).  So we are fine with the
fields that are not speled out.

But then what about the explicitly spelled out 0 for the first
field?  It is like an assignment, so by arguing that we should have
0 over there and not NULL, you are essentially arguing for

	patch.new_name = 0; /* not NULL */

aren't you?

As already pointed out downthread, sparse will catch an
initialization for an arithmetic type that is left to be NULL, but
which should have been spelled 0, when fields are reordered anyway,
so I do not think your "only when initializing the first field of
the structure with a zero value while leaving the initializer for
the remainder unspelled, we should say 0 not NULL even when the
field has pointer type" stance is not maintainable.

And I agree with you that the calloc()/memset() that fills it with
0-bit pattern is "evil-doing" as you say.  Compared to that, the
initialization rule for objects with static storage duration is
quite sane---a pointer field gets a NULL pointer and arithmetic
field gets a (positive or unsigned) zero.

I wish if we could say

	struct patch patch = {};

so that we an leave all fields to have their natural zero value like
fields in a static variable without explicit initialization do ;-)
but according to "6.7.8 Initialization / Syntax #1", the
initializer-list inside the braces must have at least one
initializer, so that won't be possible X-<.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 21:29       ` Junio C Hamano
@ 2019-07-13 22:22         ` Carlo Arenas
  2019-07-14  0:51           ` Jeff King
  2019-07-15 14:47           ` Johannes Schindelin
  2019-07-14  8:15         ` Johannes Sixt
  1 sibling, 2 replies; 21+ messages in thread
From: Carlo Arenas @ 2019-07-13 22:22 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Ramsay Jones, Thomas Gummerer, GIT Mailing-list

On Sat, Jul 13, 2019 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> I wish if we could say
>
>         struct patch patch = {};

that is actually a GNU extension that is supported by gcc and clang (at least)
and that sparse also recognizes as valid; it is also valid C++ so it might be
even supported by MSVC.

it will obviously trigger warnings if using pedantic mode and is IMHO not worth
the hassle to maintain in a portable codebase like git, but also wish could be
added to C2X

Carlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 22:22         ` Carlo Arenas
@ 2019-07-14  0:51           ` Jeff King
  2019-07-14  8:30             ` Johannes Sixt
  2019-07-15 14:47           ` Johannes Schindelin
  1 sibling, 1 reply; 21+ messages in thread
From: Jeff King @ 2019-07-14  0:51 UTC (permalink / raw)
  To: Carlo Arenas
  Cc: Junio C Hamano, Johannes Sixt, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

On Sat, Jul 13, 2019 at 03:22:03PM -0700, Carlo Arenas wrote:

> On Sat, Jul 13, 2019 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > I wish if we could say
> >
> >         struct patch patch = {};
> 
> that is actually a GNU extension that is supported by gcc and clang (at least)
> and that sparse also recognizes as valid; it is also valid C++ so it might be
> even supported by MSVC.
> 
> it will obviously trigger warnings if using pedantic mode and is IMHO not worth
> the hassle to maintain in a portable codebase like git, but also wish could be
> added to C2X

I wonder if we could come up with a definition of INIT_ZERO such that:

  struct foo bar = { INIT_ZERO };

worked everywhere. IMHO that is more readable than "{}" anyway. In
GNU-compatible compilers, it is just:

  #define INIT_ZERO

Unfortunately I can't think of a fallback that would work universally.
It cannot just be "0", as we run into the NULL thing (not to mention
when the first member is itself a struct). It does not help us to
add an explicit cast, because the type of the cast is dependent on the
context in which the macro is used. Nor do I think typeof() could save
us (if we could even assume it exists everywhere we need the fallback,
which we almost certainly can't).

But it does seem a real shame there is no way to say "zero-initialize
this struct" in C without providing at least a single member value.
Ordering struct definitions to put an arithmetic type at the start is an
OK workaround (to just let "0" work everywhere). But it does fall down
when the first element _has_ to be a struct (like, say, any user of our
hashmap.[ch] interface).

-Peff

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 21:29       ` Junio C Hamano
  2019-07-13 22:22         ` Carlo Arenas
@ 2019-07-14  8:15         ` Johannes Sixt
  1 sibling, 0 replies; 21+ messages in thread
From: Johannes Sixt @ 2019-07-14  8:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramsay Jones, Thomas Gummerer, GIT Mailing-list, Jeff King

Am 13.07.19 um 23:29 schrieb Junio C Hamano:
> I do not think this position is maintainable, especially if you
> agree with me (and everybody else, including sparse) that this is a
> bad idea:
> 
>>   struct string_list dup_it = { 0, 0, 0, 1, 0 };
> 
> The way I read "6.7.8 Initialization" (sorry, I only have committee
> draft wg14/n1124 of 2005 handy) is that
> 
> 	struct patch patch = { 0 };
> 
> has an initializer for a structure with an automatic storage
> duration, and for each of the subsequent fields of the structure
> (i.e. we ran out the initializer after dealing with that single zero
> that talks about the first field), due to "all subobjects that are
> not initialized explicitly shall be initialized implicitly the same
> as objects that have static storage duration." rule, "if it has a
> pointer type, it is initialized to a null pointer", which is exactly
> in line with your (and our) position that the first example I left
> in the above (new_name gets assigned NULL).  So we are fine with the
> fields that are not speled out.
> 
> But then what about the explicitly spelled out 0 for the first
> field?

You are putting too much meaning in the token '0' when it appears in the
token sequence '= { 0 }'. I understand this sequence as a signal to the
reader of the code that "the whole struct is to be zero-initialized".
NOT "the first member is set to zero and everything else the default
zero value".

It just so happens that the compiler does the right thing with that '0'
regardless of what type the first member has. (It even works when it is
a struct, Peff!) That zero is a null pointer constant if the first
member happens to be a pointer, i.e., the same value that is used for
all other implicitly zero-initialized pointers.

>  It is like an assignment, so by arguing that we should have
> 0 over there and not NULL, you are essentially arguing for
> 
> 	patch.new_name = 0; /* not NULL */
> 
> aren't you?

No,no. You are stretching my argument too far. I really only mean the
sequence = { 0 } as a signal. When this form of zero-initialization
becomes established, it takes away mental burden from the reader. It
need not be decomposed into its parts; there is no question of what is
it that is initialized by '0', and what happens to the rest of the
struct. It means "zero, all of it" without thinking.

> I wish if we could say
> 
> 	struct patch patch = {};
> 
> so that we an leave all fields to have their natural zero value

YES!

-- Hannes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-14  0:51           ` Jeff King
@ 2019-07-14  8:30             ` Johannes Sixt
  2019-07-15 14:46               ` Jeff King
  0 siblings, 1 reply; 21+ messages in thread
From: Johannes Sixt @ 2019-07-14  8:30 UTC (permalink / raw)
  To: Jeff King
  Cc: Carlo Arenas, Junio C Hamano, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

Am 14.07.19 um 02:51 schrieb Jeff King:
> I wonder if we could come up with a definition of INIT_ZERO such that:
> 
>   struct foo bar = { INIT_ZERO };
> 
> worked everywhere. IMHO that is more readable than "{}" anyway.

Not when = {} becomes a well-established way to express
zero-initialization. At that point, your suggested macro would obfuscate
the construct.

> But it does seem a real shame there is no way to say "zero-initialize
> this struct" in C without providing at least a single member value.

Indeed. For this reason, I'm arguing for the second-best form that
places a 0 between the braces.

> Ordering struct definitions to put an arithmetic type at the start is an
> OK workaround (to just let "0" work everywhere).
Why would you re-order members? There's nothing wrong when a pointer is
initialized by 0.

> But it does fall down
> when the first element _has_ to be a struct (like, say, any user of our
> hashmap.[ch] interface).

No, it does not. It is not necessary to spell out nested structs in the
initializer.

-- Hannes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-14  8:30             ` Johannes Sixt
@ 2019-07-15 14:46               ` Jeff King
  2019-07-15 17:30                 ` Johannes Sixt
  0 siblings, 1 reply; 21+ messages in thread
From: Jeff King @ 2019-07-15 14:46 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Carlo Arenas, Junio C Hamano, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

On Sun, Jul 14, 2019 at 10:30:27AM +0200, Johannes Sixt wrote:

> Why would you re-order members? There's nothing wrong when a pointer is
> initialized by 0.

To appease tooling like "sparse" without having to remember to do
anything specific at the point-of-use sites. I'm open to the idea that
it is not worth appeasing sparse, but this may be a simple and practical
compromise.

> > But it does fall down
> > when the first element _has_ to be a struct (like, say, any user of our
> > hashmap.[ch] interface).
> 
> No, it does not. It is not necessary to spell out nested structs in the
> initializer.

Ah, that is news to me. I know that this compiles OK with "gcc -Wall",
but is it guaranteed by the standard?

-- >8 --
#include <stdio.h>

struct outer {
	struct {
		int x;
		char *y;
	} inner;
};

int main(void)
{
	struct outer foo = { 0 };
	printf("%d %p\n", foo.inner.x, foo.inner.y);
	return 0;
}
-- 8< --

If so, then I agree that "0" is a pretty good solution, tooling like
"sparse" aside. I do agree with your sentiment that "0" can be read as
"please zero-initialize this struct" and not really as an attempt to set
an initial member pointer to zero.

-Peff

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-13 22:22         ` Carlo Arenas
  2019-07-14  0:51           ` Jeff King
@ 2019-07-15 14:47           ` Johannes Schindelin
  1 sibling, 0 replies; 21+ messages in thread
From: Johannes Schindelin @ 2019-07-15 14:47 UTC (permalink / raw)
  To: Carlo Arenas
  Cc: Junio C Hamano, Johannes Sixt, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

Hi,


On Sat, 13 Jul 2019, Carlo Arenas wrote:

> On Sat, Jul 13, 2019 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > I wish if we could say
> >
> >         struct patch patch = {};
>
> that is actually a GNU extension that is supported by gcc and clang (at least)
> and that sparse also recognizes as valid; it is also valid C++ so it might be
> even supported by MSVC.

It seems to be supported by MSVC, at least as of VS2019.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-15 14:46               ` Jeff King
@ 2019-07-15 17:30                 ` Johannes Sixt
  2019-07-15 18:15                   ` Jeff King
  0 siblings, 1 reply; 21+ messages in thread
From: Johannes Sixt @ 2019-07-15 17:30 UTC (permalink / raw)
  To: Jeff King
  Cc: Carlo Arenas, Junio C Hamano, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

Am 15.07.19 um 16:46 schrieb Jeff King:
> On Sun, Jul 14, 2019 at 10:30:27AM +0200, Johannes Sixt wrote:
>>> But it does fall down
>>> when the first element _has_ to be a struct (like, say, any user of our
>>> hashmap.[ch] interface).
>>
>> No, it does not. It is not necessary to spell out nested structs in the
>> initializer.
> 
> Ah, that is news to me. I know that this compiles OK with "gcc -Wall",
> but is it guaranteed by the standard?

Yes; see http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf,
6.7.8 §20, in particular, the sentence beginning with "Otherwise".

-- Hannes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-15 17:30                 ` Johannes Sixt
@ 2019-07-15 18:15                   ` Jeff King
  2019-07-16 19:01                     ` Junio C Hamano
  0 siblings, 1 reply; 21+ messages in thread
From: Jeff King @ 2019-07-15 18:15 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Carlo Arenas, Junio C Hamano, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

On Mon, Jul 15, 2019 at 07:30:09PM +0200, Johannes Sixt wrote:

> Am 15.07.19 um 16:46 schrieb Jeff King:
> > On Sun, Jul 14, 2019 at 10:30:27AM +0200, Johannes Sixt wrote:
> >>> But it does fall down
> >>> when the first element _has_ to be a struct (like, say, any user of our
> >>> hashmap.[ch] interface).
> >>
> >> No, it does not. It is not necessary to spell out nested structs in the
> >> initializer.
> > 
> > Ah, that is news to me. I know that this compiles OK with "gcc -Wall",
> > but is it guaranteed by the standard?
> 
> Yes; see http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf,
> 6.7.8 §20, in particular, the sentence beginning with "Otherwise".

Thanks, I didn't know about that subtlety.

I think relying on that flattening would be a terrible style choice to
use for initializing to particular values, but it makes perfect sense in
the context of using a single 0 to mean "zero-initialize the whole
struct".

That actually kind of makes me think that reordering our struct members
(to put an arithmetic type or a struct containing one at the beginning)
_might_ be worth doing as a workaround to tools like sparse. It's hacky,
but it puts the effort of dealing with this problem only in one spot
(the struct definition) instead of many (everywhere the struct is used).

But I'd be happy if we could address it in another way (e.g., convincing
sparse to stop complaining about it, or just decide it's not worth
dealing with). One other thing I haven't seen discussed in this thread:
we could actually make our preferred style be for all structs to define
a FOO_INIT initializer, like we already do for STRBUF_INIT, etc. That's
a much more heavyweight solution than what's being discussed here, but
it comes with an extra benefit: it's easy to catch and change all users
if the struct switches away from zero-initialization.

I'm on the fence on whether having non-zero initializers is a good
strategy overall.  You can always bootstrap any other on-demand setup
from the zero-initialized state, but it does sometimes lead to more
awkward code (e.g., needing an explicit call to initialized_if_needed()
in every function that works with the struct, or inverting the sense of
boolean members so that the default is always "0").

-Peff

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-15 18:15                   ` Jeff King
@ 2019-07-16 19:01                     ` Junio C Hamano
  2019-07-16 20:01                       ` Jeff King
  2019-07-17 17:23                       ` Johannes Sixt
  0 siblings, 2 replies; 21+ messages in thread
From: Junio C Hamano @ 2019-07-16 19:01 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Sixt, Carlo Arenas, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

Jeff King <peff@peff.net> writes:

> But I'd be happy if we could address it in another way (e.g., convincing
> sparse to stop complaining about it, or just decide it's not worth
> dealing with). One other thing I haven't seen discussed in this thread:
> we could actually make our preferred style be for all structs to define
> a FOO_INIT initializer, like we already do for STRBUF_INIT, etc. That's
> a much more heavyweight solution than what's being discussed here, but
> it comes with an extra benefit: it's easy to catch and change all users
> if the struct switches away from zero-initialization.

I'd rather not to go that route.  When we say

	static struct foo foo;

we are happy with the natural and trivial zero initialization of the
structure.  If we didn't have to say STRBUF_INIT, the codebase would
have been a lot nicer with less noise. Because the implementation of
the strbuf cannot take the trivial zero initialized state as a valid
one, we had to sprinkle " = STRBUF_INIT;" all over.

And that "quiet and nice" form is a moral equivalent of

	struct foo foo = { 0 };

that has been discussed in this thread.  I'd rather not to see it
turned into distinct FOO_INIT, BAR_INIT, etc. to force the reader to
think these structures all need their specific initialization and
wonder what's the reason for each of them.

One universal "struct foo foo = STRUCT_ZERO_INIT;" that is applied
to all kinds of structure I could live with (but only if we have a
good way to squelch sparse from bitching about it).  Perhaps we
could define it as "{}" for GCC, while keeping it "{ 0 }" for
others.  As I said, { 0 } is undefensible if we insist that a null
pointer must be spelled NULL and not 0 (as CodingGuidelines says),
but as long as we declare that we take "{ 0 }" as a mere convention
(like we used to use the "int foo = foo;" convention to squelch
"uninitialized but used" warnings) that is outside the purview of
language-lawyers, I am perfectly fine with it, and if it is hidden
behind a macro, that would be even better ;-)




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-16 19:01                     ` Junio C Hamano
@ 2019-07-16 20:01                       ` Jeff King
  2019-07-17 18:13                         ` Junio C Hamano
  2019-07-17 17:23                       ` Johannes Sixt
  1 sibling, 1 reply; 21+ messages in thread
From: Jeff King @ 2019-07-16 20:01 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Carlo Arenas, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

On Tue, Jul 16, 2019 at 12:01:10PM -0700, Junio C Hamano wrote:

> And that "quiet and nice" form is a moral equivalent of
> 
> 	struct foo foo = { 0 };
> 
> that has been discussed in this thread.  I'd rather not to see it
> turned into distinct FOO_INIT, BAR_INIT, etc. to force the reader to
> think these structures all need their specific initialization and
> wonder what's the reason for each of them.

I'm on the fence for that style myself. But we've definitely been
trending in that direction. Look at `git grep _INIT *.h`, many of which
are clearly zero-initializers.

I do think it's nice to be able to modify the initializers later and
feel confident that you're catching all of the users. But even then:

  - it's not like we get any kind of static warning for a
    zero-initialized variant (be it static or with a manual {0}
    initializer)

  - I know I've run into problems where code assumed memset() worked,
    but it didn't (I think diff_options was one such case).

So at best it's "feel more confident", not "feel confident". :)

> One universal "struct foo foo = STRUCT_ZERO_INIT;" that is applied
> to all kinds of structure I could live with (but only if we have a
> good way to squelch sparse from bitching about it).  Perhaps we
> could define it as "{}" for GCC, while keeping it "{ 0 }" for
> others.  As I said, { 0 } is undefensible if we insist that a null
> pointer must be spelled NULL and not 0 (as CodingGuidelines says),
> but as long as we declare that we take "{ 0 }" as a mere convention
> (like we used to use the "int foo = foo;" convention to squelch
> "uninitialized but used" warnings) that is outside the purview of
> language-lawyers, I am perfectly fine with it, and if it is hidden
> behind a macro, that would be even better ;-)

Yeah, I am OK with that. My big question is if we use "{}" for gcc (and
compatible friends), does that squelch all of the complaints from other
compilers and tools that might see the "{0}" version? In particular,
does it work for sparse?

-Peff

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-16 19:01                     ` Junio C Hamano
  2019-07-16 20:01                       ` Jeff King
@ 2019-07-17 17:23                       ` Johannes Sixt
  1 sibling, 0 replies; 21+ messages in thread
From: Johannes Sixt @ 2019-07-17 17:23 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Carlo Arenas, Ramsay Jones, Thomas Gummerer, GIT Mailing-list

Am 16.07.19 um 21:01 schrieb Junio C Hamano:
> but as long as we declare that we take "{ 0 }" as a mere convention
> [...], I am perfectly fine with it, and if it is hidden
> behind a macro, that would be even better ;-)

And I thought that "Avoid macros!" would be a welcome guideline...

I think we enter subjective territory. Let's stop here.

-- Hannes

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-16 20:01                       ` Jeff King
@ 2019-07-17 18:13                         ` Junio C Hamano
  2019-07-17 19:21                           ` Jeff King
  0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2019-07-17 18:13 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Sixt, Carlo Arenas, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

Jeff King <peff@peff.net> writes:

> ... My big question is if we use "{}" for gcc (and
> compatible friends), does that squelch all of the complaints from other
> compilers and tools that might see the "{0}" version? In particular,
> does it work for sparse?

Yeah, I agree that it is the most important question.  The best
solution is not to do the macro, use "= { 0 };" everywhere *and*
somehow arrange sparse not to complain about it.  I am not sure if
the last part is doable, though.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-17 18:13                         ` Junio C Hamano
@ 2019-07-17 19:21                           ` Jeff King
  2019-07-17 20:10                             ` Junio C Hamano
  0 siblings, 1 reply; 21+ messages in thread
From: Jeff King @ 2019-07-17 19:21 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Carlo Arenas, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

On Wed, Jul 17, 2019 at 11:13:04AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... My big question is if we use "{}" for gcc (and
> > compatible friends), does that squelch all of the complaints from other
> > compilers and tools that might see the "{0}" version? In particular,
> > does it work for sparse?
> 
> Yeah, I agree that it is the most important question.  The best
> solution is not to do the macro, use "= { 0 };" everywhere *and*
> somehow arrange sparse not to complain about it.  I am not sure if
> the last part is doable, though.

I did just check "make range-diff.sp" with this diff:

diff --git a/range-diff.c b/range-diff.c
index ba1e9a4265..481cefff3e 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -102,7 +102,7 @@ static int read_patches(const char *range, struct string_list *list)
 		}
 
 		if (starts_with(line, "diff --git")) {
-			struct patch patch = { 0 };
+			struct patch patch = { };
 			struct strbuf root = STRBUF_INIT;
 			int linenr = 0;
 

and it seems OK. So presumably we could just lump sparse into the list
of gcc-compatible platforms, and it would work. But it does require the
macro still for other hosts.

Other than that, our options seem to be:

  1. Live with it. IIRC we're already not sparse-clean, and Ramsay
     mostly looks at the diff to find new problems.

  2. Pass -Wno-non-pointer-null to sparse. Unfortunately that also
     disables more useful warnings (like passing "0" instead of NULL to
     a function).

  3. Switch to NULL here, and adhere to that going forward.

-Peff

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
  2019-07-17 19:21                           ` Jeff King
@ 2019-07-17 20:10                             ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2019-07-17 20:10 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Sixt, Carlo Arenas, Ramsay Jones, Thomas Gummerer,
	GIT Mailing-list

Jeff King <peff@peff.net> writes:

> Other than that, our options seem to be:
>
>   1. Live with it. IIRC we're already not sparse-clean, and Ramsay
>      mostly looks at the diff to find new problems.

OK.

>   2. Pass -Wno-non-pointer-null to sparse. Unfortunately that also
>      disables more useful warnings (like passing "0" instead of NULL to
>      a function).

Non starter, I am afraind.  The loss outweighs.

>   3. Switch to NULL here, and adhere to that going forward.

This does not sound too bad.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 22:03 [PATCH] range-diff: fix some 'hdr-check' and sparse warnings Ramsay Jones
2019-07-12  5:21 ` Johannes Sixt
2019-07-12 16:44   ` Junio C Hamano
2019-07-13 10:44     ` Johannes Sixt
2019-07-13 12:18       ` Johannes Sixt
2019-07-13 12:56       ` Carlo Arenas
2019-07-13 21:29       ` Junio C Hamano
2019-07-13 22:22         ` Carlo Arenas
2019-07-14  0:51           ` Jeff King
2019-07-14  8:30             ` Johannes Sixt
2019-07-15 14:46               ` Jeff King
2019-07-15 17:30                 ` Johannes Sixt
2019-07-15 18:15                   ` Jeff King
2019-07-16 19:01                     ` Junio C Hamano
2019-07-16 20:01                       ` Jeff King
2019-07-17 18:13                         ` Junio C Hamano
2019-07-17 19:21                           ` Jeff King
2019-07-17 20:10                             ` Junio C Hamano
2019-07-17 17:23                       ` Johannes Sixt
2019-07-15 14:47           ` Johannes Schindelin
2019-07-14  8:15         ` Johannes Sixt

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox