Message-ID: <87lewtfzfj.ffs@tglx>
Date:   Tue, 29 Mar 2022 01:08:16 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Benjamin Stürz <benni@...erz.xyz>, andrew@...n.ch
Cc:     sebastian.hesselbarth@...il.com, gregory.clement@...tlin.com,
        linux@...linux.org.uk, linux@...tec.co.uk, krzk@...nel.org,
        alim.akhtar@...sung.com, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, robert.moore@...el.com,
        rafael.j.wysocki@...el.com, lenb@...nel.org, 3chas3@...il.com,
        laforge@...monks.org, arnd@...db.de, gregkh@...uxfoundation.org,
        mchehab@...nel.org, tony.luck@...el.com, james.morse@....com,
        rric@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl,
        mike.marciniszyn@...nelisnetworks.com,
        dennis.dalessandro@...nelisnetworks.com, jgg@...pe.ca,
        pali@...nel.org, dmitry.torokhov@...il.com, isdn@...ux-pingi.de,
        benh@...nel.crashing.org, fbarrat@...ux.ibm.com, ajd@...ux.ibm.com,
        davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        nico@...xnic.net, loic.poulain@...aro.org, kvalo@...nel.org,
        pkshih@...ltek.com, bhelgaas@...gle.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org, linux-ia64@...r.kernel.org,
        linux-acpi@...r.kernel.org, devel@...ica.org,
        linux-atm-general@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-edac@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-rdma@...r.kernel.org, linux-input@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, linux-media@...r.kernel.org,
        wcn36xx@...ts.infradead.org, linux-wireless@...r.kernel.org,
        linux-pci@...r.kernel.org,
        Benjamin Stürz <benni@...erz.xyz>
Subject: Re: [PATCH 04/22] x86: Replace comments with C99 initializers

Benjamin,

On Sat, Mar 26 2022 at 17:58, Benjamin Stürz wrote:

> This replaces comments with C99's designated
> initializers because the kernel supports them now.

the kernel has used designated array initializers for a very long time
simply because the kernel did not use pure C89 but C89 with GNU
extensions, i.e. -std=gnu89, which include designated array
initializers. GCC supports this since 1998 with =gnu89, so 'now' is
more than slightly off.

Explicit value assignment to enum constants are a different story. They
are neither designated initializers nor new in C99. The following
paragraph from the standard has not been changed since C89:

   "The identifiers in an enumerator list are declared as constants that
    have type int and may appear wherever such are permitted. An
    enumerator with = defines its enumeration constant as the value of
    the constant expression. If the first enumerator has no =, the value
    of its enumeration constant is 0. Each subsequent enumerator with no
    = defines its enumeration constant as the value of the constant
    expression obtained by adding 1 to the value of the previous
    enumeration constant. (The use of enumerators with = may produce
    enumeration constants with values that duplicate other values in the
    same enumeration.)"

Please make sure that your changelogs are factual. Making uninformed
claims is not helping your cause.

The most important part is the WHY:

    Why is the current code suboptimal?

    Why is the proposed change making it better, more correct, less
    error prone?

If you can't come up with proper technical answers for these questions
then why should it be changed?

>  enum regnames {
> -	GDB_AX,			/* 0 */
> +	GDB_AX = 0,

Linear enums without value assignment like here are not a problem at
all. Simply because they are well defined and well understood. See the
above quote of the standard.

Whether the explicit assignment is an improvement over the trailing
comment or not is a matter of taste and preference. There is absolutely
_zero_ technical advantage in using explicit value assignments in _this_
case and neither in many other cases of your series.

Also completely removing the comments here loses information:

> -	GDB_PC,			/* 8 also known as eip */
> -	GDB_PS,			/* 9 also known as eflags */

Can you map _PC to EIP and _PS to EFLAGS? I can't without digging
deep...

>  static const char *const mtrr_strings[MTRR_NUM_TYPES] =
>  {
> -	"uncachable",		/* 0 */
> -	"write-combining",	/* 1 */
> -	"?",			/* 2 */
> -	"?",			/* 3 */
> -	"write-through",	/* 4 */
> -	"write-protect",	/* 5 */
> -	"write-back",		/* 6 */
> +	[0] = "uncachable",
> +	[1] = "write-combining",
> +	[2] = "?",
> +	[3] = "?",
> +	[4] = "write-through",
> +	[5] = "write-protect",
> +	[6] = "write-back",

Again, while not supported in C89, the kernel uses designators in array
initializers for a very long time...

Linear array initializers like the mtrr strings are not a real problem
simply because there is no correlation and the code using the array
still has to make sure that the index into the array is what it expects
to be the content. Changing it from C89 automatic to explicit C99
designators does not help there at all.

It becomes a different story if you combine [enum] constants and arrays
and use the constants in code because then the change to the constants
will immediately be reflected in the array initializer. I.e. for this
case:

enum foo {
     BAR,
     BAZ,
     RAB,
     ZAR,
};

char foobar[] = {
     "bar",
     "baz",
     "rab",
     "zar",
};

it makes a difference if someone does:

  enum foo {
     BAR,
     BAZ,
+    MOO,
     RAB,
     ZAR,
  };

because then the related array initializer is obviously out of
order. With:

char *foobar[] = {
     [BAR] = "bar",
     [BAZ] = "baz",
     [RAB] = "rab",
     [ZAR] = "zar",
};

the existing values are still in the same place, just the newly added
value is a NULL pointer. It also does not matter when the fixup for the
missing array entry becomes:

  char *foobar[] = {
     [BAR] = "bar",
     [BAZ] = "baz",
     [RAB] = "rab",
     [ZAR] = "zar",
+    [MOO] = "moo",     
  };

because the compiled result is still in enum order. While doing

  char foobar[] = {
     "bar",
     "baz",
     "rab",
     "zar",
+    "moo",
  };

would be blantantly wrong. See?

But that does not apply to any of your proposed changes.

So you really need to look at things and not just throw a mechanical
change scheme at it, which results even in failures like reported by
the 0-day robot against patch 10/22.

That said, I'm not completely opposed to those changes, but you really
have to come up with good reasons why they make sense aside of
cosmetical reasons.

Btw, the really important change regarding initializers between C89 and
C99 was the ability to initialize struct members explicitly.

In C89 the only way to initialize a struct was

   = { a, b, c, d }

which was way more error prone than the enum/array initializers. The
dangerous part or C89 struct initializers are changes to the struct
unless the change of the struct is actually triggering a type
mismatch.

But even that needs some serious inspection whether there is confusion
potential or not. The harmless example is a file local:

struct foo {
       unsigned int      id;
       unsigned int      flags;
};

and the C89 style initializer:

static struct foo[] {
       { ID_A, 0x01 },
       { ID_B, 0x02 },
       { ID_C, 0x01 },
};

which has a very low confusion potential simply because it's scope is
file local and well defined and unlikely to change.

A globally used structure is a different problem especially when it
becomes necessary to insert a new struct member in the middle instead of
appending it, changing the order, removing a member... That ends up in a
hard to diagnose disaster with C89 style unnamed initializers pretty
fast.

Ideally C89 style unnamed struct initializers should not be used at all,
but again it's a matter of common sense and justification whether it's
worth to change it just because.

Thanks,

        tglx

Powered by blists - more mailing lists