Re: [PATCH] Fix -Wundef for FEATURE_INDEX_1.

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/30/2014 01:55 PM, Roland McGrath wrote:
>> This is the simplest patch I can come up with to fix FEATURE_INDEX_1
>> -Wundef warnings. The constant definition is duplicated because we
>> can't use the enum constant in assembly nor in the other macros.
> 
> What does "nor in the other macros" mean?  Of course a macro can be
> based on arithmetic plus use of another constant that is usable in the
> contexts where the macro is used.  enum constants cannot be used in
> assembly, nor in #if contexts.  Did you mean to say that the other
> macros are used in #if contexts and therefore must not use enum
> constants?

Correct.

>> The down side is the slight duplication. I considered adding another
>> macro e.g.
>> #define __FEATURE_INDEX_1 0
>> ...
>> # define FEATURE_INDEX_1 __FEATURE_INDEX_1
>> ...
>> However, that just seemed ugly, so I left the duplication.
> 
> When "duplication" concretely means two instances of one macro whose
> value is zero, then it seems less ugly than many other things.  But in
> the general case, duplication is worse than most other things.
> 
> In this situation, the question is, what are we getting from
> FEATURE_INDEX_* being enum constants rather than macros at all?

Good point. Nothing really.

The enum constant use was likely a first step at making this
typo-proof, but you need to convert everything and use sym
files for assembly. Thus I find this easier and equally typo-proof.

> This is internal-only code, so ease of maintenance is really the only
> issue that we care much about.  The general reason to have enum
> constants for things is so that you can use them in the debugger rather
> than either looking up values in the source by hand or relying on -g3
> macro info.  That is far less useful than the general case when its an
> anonymous enum, so there is no type to which you can cast an integer
> value to get the symbolic name trivially in gdb.  Hence, literally the
> only benefit is typing FEATURE_INDEX_1 in gdb.  If we cared about that,
> we'd make all the macros in init-arch.h be enum constants.  So I think
> we just aren't getting anything worthwhile from having it be an enum.
> Let's eliminate the duplication by just having one unconditional #define.

Done. I've removed only the FEATURE_INDEX enum which is the only
problematic case for -Wundef.

This fixes all FEATURE_INDEX* warnings.

No regressions on x86-64 (and you'd notice, I messed this up once
and the whole testsuite falls over because it breaks ifuncs).

2014-05-01  Carlos O'Donell  <carlos@redhat.com>

	* sysdeps/x86_64/multiarch/init-arch.h:	Define FEATURE_INDEX_1 to 0.
	[!__ASSEMBLER__]: Remove anonymous enum for FEATURE_INDEX_*.

diff --git a/sysdeps/x86_64/multiarch/init-arch.h b/sysdeps/x86_64/multiarch/init-arch.h
index 813b6de..e4d265d 100644
--- a/sysdeps/x86_64/multiarch/init-arch.h
+++ b/sysdeps/x86_64/multiarch/init-arch.h
@@ -47,6 +47,12 @@
 #define bit_XMM_state  (1 << 1)
 #define bit_YMM_state  (2 << 1)
 
+/* The integer bit array index for the first set of internal feature bits.  */
+# define FEATURE_INDEX_1 0
+
+/* The current maximum size of the feature integer bit array.  */
+# define FEATURE_INDEX_MAX 1
+
 #ifdef __ASSEMBLER__
 
 # include <ifunc-defines.h>
@@ -82,13 +88,6 @@ enum
     COMMON_CPUID_INDEX_MAX
   };
 
-enum
-  {
-    FEATURE_INDEX_1 = 0,
-    /* Keep the following line at the end.  */
-    FEATURE_INDEX_MAX
-  };
-
 extern struct cpu_features
 {
   enum cpu_features_kind
---

Cheers,
Carlos.