Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
Am 12.07.19 um 18:44 schrieb Junio C Hamano:
> Johannes Sixt 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
Am 13.07.19 um 12:44 schrieb Johannes Sixt:
> Am 12.07.19 um 18:44 schrieb Junio C Hamano:
>> Johannes Sixt 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
On Sat, Jul 13, 2019 at 3:46 AM Johannes Sixt 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
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
Johannes Sixt 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
On Sat, Jul 13, 2019 at 2:29 PM Junio C Hamano 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
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 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
[PATCH] read-cache.c: do not die if mmap fails
do_read_index() mmaps the index, or tries to die with an error message
on failure. It should call xmmap_gently(), which returns MAP_FAILED,
rather than xmmap(), which dies with its own error message.
An easy way to cause this mmap to fail is by setting $GIT_INDEX_FILE to
a path to a directory and then invoking any command that reads from the
index.
Signed-off-by: Varun Naik
---
I believe this is the only place that calls xmmap() when it should be
calling xmmap_gently(). There is a related recent commit 3203566a71
("Use xmmap_gently instead of xmmap in use_pack", 2019-05-16).
read-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 22e7b9944e..4e30dafa9d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2140,7 +2140,7 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist)
if (mmap_size < sizeof(struct cache_header) + the_hash_algo->rawsz)
die(_("%s: index file smaller than expected"), path);
- mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ mmap = xmmap_gently(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (mmap == MAP_FAILED)
die_errno(_("%s: unable to map index file"), path);
close(fd);
--
2.22.0
