| Message ID | 20220326165909.506926-4-benni@stuerz.xyz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/22] orion5x: Replace comments with C99 initializers | expand |
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
diff --git a/arch/x86/include/asm/kgdb.h b/arch/x86/include/asm/kgdb.h index aacaf2502bd2..a9161e2f8b63 100644 --- a/arch/x86/include/asm/kgdb.h +++ b/arch/x86/include/asm/kgdb.h @@ -25,52 +25,52 @@ */ #ifdef CONFIG_X86_32 enum regnames { - GDB_AX, /* 0 */ - GDB_CX, /* 1 */ - GDB_DX, /* 2 */ - GDB_BX, /* 3 */ - GDB_SP, /* 4 */ - GDB_BP, /* 5 */ - GDB_SI, /* 6 */ - GDB_DI, /* 7 */ - GDB_PC, /* 8 also known as eip */ - GDB_PS, /* 9 also known as eflags */ - GDB_CS, /* 10 */ - GDB_SS, /* 11 */ - GDB_DS, /* 12 */ - GDB_ES, /* 13 */ - GDB_FS, /* 14 */ - GDB_GS, /* 15 */ + GDB_AX = 0, + GDB_CX = 1, + GDB_DX = 2, + GDB_BX = 3, + GDB_SP = 4, + GDB_BP = 5, + GDB_SI = 6, + GDB_DI = 7, + GDB_PC = 8, /* also known as eip */ + GDB_PS = 9, /* also known as eflags */ + GDB_CS = 10, + GDB_SS = 11, + GDB_DS = 12, + GDB_ES = 13, + GDB_FS = 14, + GDB_GS = 15, }; #define GDB_ORIG_AX 41 #define DBG_MAX_REG_NUM 16 #define NUMREGBYTES ((GDB_GS+1)*4) #else /* ! CONFIG_X86_32 */ enum regnames { - GDB_AX, /* 0 */ - GDB_BX, /* 1 */ - GDB_CX, /* 2 */ - GDB_DX, /* 3 */ - GDB_SI, /* 4 */ - GDB_DI, /* 5 */ - GDB_BP, /* 6 */ - GDB_SP, /* 7 */ - GDB_R8, /* 8 */ - GDB_R9, /* 9 */ - GDB_R10, /* 10 */ - GDB_R11, /* 11 */ - GDB_R12, /* 12 */ - GDB_R13, /* 13 */ - GDB_R14, /* 14 */ - GDB_R15, /* 15 */ - GDB_PC, /* 16 */ - GDB_PS, /* 17 */ - GDB_CS, /* 18 */ - GDB_SS, /* 19 */ - GDB_DS, /* 20 */ - GDB_ES, /* 21 */ - GDB_FS, /* 22 */ - GDB_GS, /* 23 */ + GDB_AX = 0, + GDB_BX = 1, + GDB_CX = 2, + GDB_DX = 3, + GDB_SI = 4, + GDB_DI = 5, + GDB_BP = 6, + GDB_SP = 7, + GDB_R8 = 8, + GDB_R9 = 9, + GDB_R10 = 10, + GDB_R11 = 11, + GDB_R12 = 12, + GDB_R13 = 13, + GDB_R14 = 14, + GDB_R15 = 15, + GDB_PC = 16, /* also known as eip */ + GDB_PS = 17, /* also known as eflags */ + GDB_CS = 18, + GDB_SS = 19, + GDB_DS = 20, + GDB_ES = 21, + GDB_FS = 22, + GDB_GS = 23, }; #define GDB_ORIG_AX 57 #define DBG_MAX_REG_NUM 24 diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c index a5c506f6da7f..7664a672f414 100644 --- a/arch/x86/kernel/cpu/mtrr/if.c +++ b/arch/x86/kernel/cpu/mtrr/if.c @@ -18,13 +18,13 @@ 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", }; const char *mtrr_attrib_to_str(int x)
This replaces comments with C99's designated initializers because the kernel supports them now. Signed-off-by: Benjamin Stürz <benni@stuerz.xyz> --- arch/x86/include/asm/kgdb.h | 80 +++++++++++++++++------------------ arch/x86/kernel/cpu/mtrr/if.c | 14 +++--- 2 files changed, 47 insertions(+), 47 deletions(-)