| Message ID | e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | range-diff: fix some 'hdr-check' and sparse warnings | expand |
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
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?
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
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 >
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
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-<.
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
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
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
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
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
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
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
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
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 ;-)
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
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
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.
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
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.
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.
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
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;