git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Sixt <j6t@kdbg.org>
Cc: Ramsay Jones <ramsay@ramsayjones.plus.com>,
	Thomas Gummerer <t.gummerer@gmail.com>,
	GIT Mailing-list <git@vger.kernel.org>
Subject: Re: [PATCH] range-diff: fix some 'hdr-check' and sparse warnings
Date: Sat, 13 Jul 2019 14:29:05 -0700	[thread overview]
Message-ID: <xmqq1rytpqse.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <ec635d0d-00ca-2419-3c1a-9b0343b46daa@kdbg.org> (Johannes Sixt's message of "Sat, 13 Jul 2019 12:44:20 +0200")

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


  parent reply	other threads:[~2019-07-13 21:29 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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
2020-10-02 17:03             ` Junio C Hamano
2020-10-04 18:35               ` Johannes Schindelin
2019-07-14  8:15         ` Johannes Sixt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq1rytpqse.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=ramsay@ramsayjones.plus.com \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).