[PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Ramsay Jones]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Sixt]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Sixt]

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
> 

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Carlo Arenas]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Junio C Hamano]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Carlo Arenas]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Jeff King]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Sixt]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Sixt]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Jeff King]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Schindelin]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Sixt]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Jeff King]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Junio C Hamano]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Jeff King]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Sixt]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Junio C Hamano]

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.

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Jeff King]

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

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Junio C Hamano]

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.

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

In <nycvar.QRO.7.76.6.2010021555290.50@tvgsbejvaqbjf.bet> on Fri, 2
Oct 2020 15:57:45 +0200 (CEST), it was said:

> Before you further split it up, I encourage you to include these patches
> without which the CI builds will continue to fail (Junio, could I ask you
> to either cherry-pick them from https://github.com/git-for-windows/git's
> shears/seen branch, or apply them from the mbox?):
>
> -- snipsnap --
> From e485e006f34922439f2e971a1c5c38b8ca56c011 Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Wed, 30 Sep 2020 14:46:59 +0200
> Subject: [PATCH 1/3] fixup??? reftable: rest of library
>
> 	struct abc x = {}
>
> is a GNUism.

Perhaps VS2020 no longer allows it?

;-)

Jokes aside, I think we agreed in that old thread I am responding to
that "= { 0 }" was the way to go, so let's keep doing so.

Thanks.

Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings

[Johannes Schindelin]

Hi Junio,

On Fri, 2 Oct 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.
>
> In <nycvar.QRO.7.76.6.2010021555290.50@tvgsbejvaqbjf.bet> on Fri, 2
> Oct 2020 15:57:45 +0200 (CEST), it was said:
>
> > Before you further split it up, I encourage you to include these patches
> > without which the CI builds will continue to fail (Junio, could I ask you
> > to either cherry-pick them from https://github.com/git-for-windows/git's
> > shears/seen branch, or apply them from the mbox?):
> >
> > -- snipsnap --
> > From e485e006f34922439f2e971a1c5c38b8ca56c011 Mon Sep 17 00:00:00 2001
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Date: Wed, 30 Sep 2020 14:46:59 +0200
> > Subject: [PATCH 1/3] fixup??? reftable: rest of library
> >
> > 	struct abc x = {}
> >
> > is a GNUism.
>
> Perhaps VS2020 no longer allows it?
>
> ;-)

I suspect that it does allow it, but only when compiling C++, not when
compiling C. At least in my hands, it failed to compiler that construct.

> Jokes aside, I think we agreed in that old thread I am responding to
> that "= { 0 }" was the way to go, so let's keep doing so.

Yep, we're on the same page.

Ciao,
Dscho