archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
To: "Martin Ågren" <>
Cc: Git Mailing List <>,
	Junio C Hamano <>, Jeff King <>
Subject: Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers
Date: Thu, 01 Jul 2021 18:08:01 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thu, Jul 01 2021, Martin Ågren wrote:

> On Thu, 1 Jul 2021 at 12:54, Ævar Arnfjörð Bjarmason <> wrote:
>> Move *_INIT macros I'll use in a subsequent commits to designated
>> initializers. This isn't required for those follow-up changes, but
>> since I'm changing things in this are let's use the modern pattern
>> over the old one while we're at it.
> s/are/area/, maybe even s/area/area,/ on top.

Will fix.

>> +#define CREDENTIAL_INIT { \
>> +       .helpers = STRING_LIST_INIT_DUP, \
>> +}
> I've verified that all of these designated initializers assign the exact
> same fields before and after this commit, e.g., `helpers` actually is
> what comes first in the struct.


>> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
>> -#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>> +#define STRING_LIST_INIT_NODUP { 0 }
>> +#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }
> This "NODUP" one is a bit of an odd case though. You don't actually
> change it to use designated initializers, as the patch says. Instead you
> change it in a slightly different way. In a sense, you assign the
> literal zero not to strdup_strings, but to the first field, which is a
> pointer, where I would have expected NULL.
> I think there's been some recent-ish "we can assign `{ 0 }` even if the
> first field is a pointer, because it's idiomatic and works out fine"
> even if assigning 0 to a pointer looks a bit odd. So it might not be
> wrong as such, but it doesn't match what the commit message claims, and
> I think it would be more clear to use `{ .strdup_strings = 0 }` here:
> We're explicitly interested in *not* duplicating the strings. It's not
> just some default "let's leave everything as zero/NULL".

Yes I'm really conflating a few different things here:

 1. Whether to use designated initializers.

 2. Whether to skip explicit initialization, i.e. not enumerate
    everything, and have the ones not mentioned initializes as if they
    had static storage duration.

 3. Using the 0 null pointer constant instead of NULL while doing #2.

 4. Using our in-codebase idiom of only setting any boolean flags during
    initialization if we're setting them to true, 

So I think in this case just keeping it as "{ 0 }" makes the most
sense. Even better would be just a "{}", but that's a GNU-only

I think "{ NULL }" is just a distraction, i.e. it's better to use "{ 0
}" consistently everywhere. C guarantees the same-ness of the two, and
the point is to initialize the struct as if though it were done with a
memset() to 0, not mentally keep track of whether the first member is an
int, pointer or char * (in one other case we use '\0').

  parent reply	other threads:[~2021-07-01 16:28 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-01 10:51 [PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic Ævar Arnfjörð Bjarmason
2021-07-01 10:51 ` [PATCH 1/5] *.h: move some *_INIT to designated initializers Ævar Arnfjörð Bjarmason
2021-07-01 15:00   ` Martin Ågren
2021-07-01 15:56     ` Junio C Hamano
2021-07-01 16:08     ` Ævar Arnfjörð Bjarmason [this message]
2021-07-01 10:51 ` [PATCH 2/5] *.c *_init(): define in terms of corresponding *_INIT macro Ævar Arnfjörð Bjarmason
2021-07-01 10:51 ` [PATCH 3/5] dir.[ch]: replace dir_init() with DIR_INIT Ævar Arnfjörð Bjarmason
2021-07-01 10:51 ` [PATCH 4/5] string-list.[ch]: add a string_list_init_{nodup,dup}() Ævar Arnfjörð Bjarmason
2021-07-01 10:51 ` [PATCH 5/5] string-list.h users: change to use *_{nodup,dup}() Ævar Arnfjörð Bjarmason
2021-07-01 16:13 ` [PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic Jeff King

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:

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

  git send-email \ \ \ \ \ \ \

* 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.
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 NNTP newsgroup(s).