Re: [PATCH] use enum values instead of integer constants

[Nick Clifton <nickc at redhat dot com>]

Hi Trev,

> Its easier to understand what's going on if the value is a member of the
> variables type.

Not always...

> Some of these cases are just in sentinal array elements and it
> doesn't matter much, but it doesn't really hurt anything, and it makes it
> easier to find the cases where it does matter.

I disagree.  I think that the sentinel values should remain as numeric zeros
and not enum values.  This is because their value is deliberately meant to 
indicate the end of a list/array, and not be taken as being an active member
of that list/array.  So, for example:

> +++ b/gas/config/tc-aarch64.c
> @@ -7987,7 +7987,7 @@ struct aarch64_option_abi_value_table
>   static const struct aarch64_option_abi_value_table aarch64_abis[] = {
>     {"ilp32",		AARCH64_ABI_ILP32},
>     {"lp64",		AARCH64_ABI_LP64},
> -  {NULL,		0}
> +  {NULL,	AARCH64_ABI_LP64	}
>   };

I think that this is wrong, since the last entry in the table is not
an ABI, it is a sentinel.  Plus it is definitely not associated with the 
LP64 ABI - an earlier entry covers that.  

If you really want to use enums, but also maintain the sentinel status,
then you need to add a new enum value specifically for that purpose.  IE:

  enum aarch64_abi_type
  {
    AARCH64_ABI_UNKNOWN = 0,
    AARCH64_ABI_LP64 = 1,
    AARCH64_ABI_ILP32 = 2
  };

  static const struct aarch64_option_abi_value_table aarch64_abis[] = {
    {"ilp32",		AARCH64_ABI_ILP32},
    {"lp64",		AARCH64_ABI_LP64},
    {NULL,		AARCH64_ABI_UNKNOWN}
  };

Alternatively, since there is really no need for this sentinel in the
first place, you could leave the enum alone and change the code to:

  static const struct aarch64_option_abi_value_table aarch64_abis[] = {
    {"ilp32",		AARCH64_ABI_ILP32},
    {"lp64",		AARCH64_ABI_LP64}
  };

  [...]

  for (i = 0, opt = aarch64_abis; i < ARRAY_SIZE (aarch64_abis); i++, opt++)

or even:

  for (i = ARRAY_SIZE (aarch64_abis); --i;)
    {
      opt = aarch64_abis[i];

and just let the compiler optimize this itself.


> +++ b/gas/config/tc-i386.c
> @@ -4657,7 +4657,7 @@ match_template (void)
>     unsigned int j;
>     unsigned int found_cpu_match;
>     unsigned int check_register;
> -  enum i386_error specific_error = 0;
> +  enum i386_error specific_error = operand_size_mismatch;

I think that this is another case where a new enum value is required.
Then the assignment could be:

      enum i386_error specific_error = no_error;

Which makes more sense.


> +++ b/gas/config/tc-visium.c
> @@ -275,7 +275,7 @@ static struct visium_arch_option_table visium_archs[] =
>     {"mcm",   VISIUM_ARCH_MCM},
>     {"gr5",   VISIUM_ARCH_MCM},
>     {"gr6",   VISIUM_ARCH_GR6},
> -  {NULL, 0}
> +  {NULL, VISIUM_ARCH_DEF}
>   };

Another candidate for sentinel removal.  If you are going to keep
the sentinel, then the arch_val field should really be set to something
like VISIUM_ARCH_UNKNOWN.


> +++ b/opcodes/mcore-opc.h
> @@ -206,6 +206,6 @@ const mcore_opcode_info mcore_table[] =
>     { "rori",	RSI,	0,	0x3800 },
>     { "rotri",	RSI,    0,	0x3800 },
>     { "nop",	O0,     0,	0x1200 },  /* mov r0, r0 */
> -  { 0,		0,	0,      0 }
> +  { 0,		O0,	0,      0 }
>   };
>   #endif

I am sorry, but I just do not like this kind of thing.  The sentinel
value should not have a valid opcode class.  It should be set to a
class that is specifically intended to indicate that the class is 
unknown/invalid or to 0, indicating that the opclass field of the 
sentinel is never going to be used.


Cheers
  Nick