[v2,5/5] CodingGuidelines: recommend against unportable C99 struct syntax

Commit Message

Junio C Hamano Oct. 10, 2022, 8:38 p.m. UTC

From: Ævar Arnfjörð Bjarmason <[e-mail]>

Per 33665d98e6b (reftable: make assignments portable to AIX xlc
v12.01, 2022-03-28) forms like ".a.b = *c" can be replaced by using
".a = { .b = *c }" instead.

We'll probably allow these sooner than later, but since the workaround
is trivial let's note it among the C99 features we'd like to hold off
on for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---
 Documentation/CodingGuidelines | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Jeff King Oct. 11, 2022, 12:09 a.m. UTC | #1

On Mon, Oct 10, 2022 at 01:38:00PM -0700, Junio C Hamano wrote:

> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 9598b45f7e..cbe0377699 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -242,6 +242,10 @@ For C programs:
>       printf("%"PRIuMAX, (uintmax_t)v).  These days the MSVC version we
>       rely on supports %z, but the C library used by MinGW does not.
>  
> +   . Shorthand like ".a.b = *c" in struct assignments is known to trip
> +     up an older IBM XLC version, use ".a = { .b = *c }" instead. See
> +     the 33665d98e6b portability fix from mid-2022.

FWIW, the use of the word "assignment" here left me scratching my head.
Reading 33665d98e6b, it is about struct initialization.

C99 does allow assignment from a compound literal, like:

  foo = (struct some_struct){ .a = 1, .b = 2 };

but I don't think we have any examples in the current code (and I think
we'd consider it a different feature for purposes of testing a weather
balloon).

-Peff

Junio C Hamano Oct. 11, 2022, 12:26 a.m. UTC | #2

Jeff King <[e-mail]> writes:

> On Mon, Oct 10, 2022 at 01:38:00PM -0700, Junio C Hamano wrote:
>
>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
>> index 9598b45f7e..cbe0377699 100644
>> --- a/Documentation/CodingGuidelines
>> +++ b/Documentation/CodingGuidelines
>> @@ -242,6 +242,10 @@ For C programs:
>>       printf("%"PRIuMAX, (uintmax_t)v).  These days the MSVC version we
>>       rely on supports %z, but the C library used by MinGW does not.
>>  
>> +   . Shorthand like ".a.b = *c" in struct assignments is known to trip
>> +     up an older IBM XLC version, use ".a = { .b = *c }" instead. See
>> +     the 33665d98e6b portability fix from mid-2022.
>
> FWIW, the use of the word "assignment" here left me scratching my head.
> Reading 33665d98e6b, it is about struct initialization.

Thanks, I missed that confusion in the new description.  Perhaps
another round of reroll would make the series polished enough?

Jeff King Oct. 11, 2022, 12:39 a.m. UTC | #3

On Mon, Oct 10, 2022 at 05:26:36PM -0700, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > On Mon, Oct 10, 2022 at 01:38:00PM -0700, Junio C Hamano wrote:
> >
> >> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> >> index 9598b45f7e..cbe0377699 100644
> >> --- a/Documentation/CodingGuidelines
> >> +++ b/Documentation/CodingGuidelines
> >> @@ -242,6 +242,10 @@ For C programs:
> >>       printf("%"PRIuMAX, (uintmax_t)v).  These days the MSVC version we
> >>       rely on supports %z, but the C library used by MinGW does not.
> >>  
> >> +   . Shorthand like ".a.b = *c" in struct assignments is known to trip
> >> +     up an older IBM XLC version, use ".a = { .b = *c }" instead. See
> >> +     the 33665d98e6b portability fix from mid-2022.
> >
> > FWIW, the use of the word "assignment" here left me scratching my head.
> > Reading 33665d98e6b, it is about struct initialization.
> 
> Thanks, I missed that confusion in the new description.  Perhaps
> another round of reroll would make the series polished enough?

I read through the rest of it fairly lightly, but I didn't see have any
other complaints (and the overall goal of documenting our use of
compiler features is a great one).

-Peff

Ævar Arnfjörð Bjarmason Oct. 11, 2022, 8:30 a.m. UTC | #4

On Mon, Oct 10 2022, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
>
>> On Mon, Oct 10, 2022 at 01:38:00PM -0700, Junio C Hamano wrote:
>>
>>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
>>> index 9598b45f7e..cbe0377699 100644
>>> --- a/Documentation/CodingGuidelines
>>> +++ b/Documentation/CodingGuidelines
>>> @@ -242,6 +242,10 @@ For C programs:
>>>       printf("%"PRIuMAX, (uintmax_t)v).  These days the MSVC version we
>>>       rely on supports %z, but the C library used by MinGW does not.
>>>  
>>> +   . Shorthand like ".a.b = *c" in struct assignments is known to trip
>>> +     up an older IBM XLC version, use ".a = { .b = *c }" instead. See
>>> +     the 33665d98e6b portability fix from mid-2022.
>>
>> FWIW, the use of the word "assignment" here left me scratching my head.
>> Reading 33665d98e6b, it is about struct initialization.
>
> Thanks, I missed that confusion in the new description.  Perhaps
> another round of reroll would make the series polished enough?

I could re-roll it, but I also see you extensively fixed it up v.s. my
version. I think a re-roll here would just be
s/assignments/initializations/, so if you wanted to squash that in to
your already extensive squashes...

...or I could also re-roll it, up to you. Just let me know what you'd
prefer, thanks!

Junio C Hamano Oct. 11, 2022, 3:01 p.m. UTC | #5

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

>>> FWIW, the use of the word "assignment" here left me scratching my head.
>>> Reading 33665d98e6b, it is about struct initialization.
>>
>> Thanks, I missed that confusion in the new description.  Perhaps
>> another round of reroll would make the series polished enough?
>
> I could re-roll it, but I also see you extensively fixed it up v.s. my
> version. I think a re-roll here would just be
> s/assignments/initializations/, so if you wanted to squash that in to
> your already extensive squashes...

I do not think there has been that much squashing.  Unless I took a
wrong range-diff in [0/5] of the series, the fix-ups are to reword
the proposed log message for [3/5] and to remove an unrelated hunk
that does "now we allow it, let's use it" which is better done
outside the topic with its own justification (and "it once used to
be there" is not a good justification) in [3/5], plus a three-byte
grammofix in [4/5].

So throwing "assignments" -> "initializations" into the mix is not
too much work for me.

Unless I forget, that is ;-)

Patch

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 9598b45f7e..cbe0377699 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -242,6 +242,10 @@  For C programs:
      printf("%"PRIuMAX, (uintmax_t)v).  These days the MSVC version we
      rely on supports %z, but the C library used by MinGW does not.
 
+   . Shorthand like ".a.b = *c" in struct assignments is known to trip
+     up an older IBM XLC version, use ".a = { .b = *c }" instead. See
+     the 33665d98e6b portability fix from mid-2022.
+
  - Variables have to be declared at the beginning of the block, before
    the first statement (i.e. -Wdeclaration-after-statement).