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

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

On Thu, Apr 21, 2016 at 10:55:22AM +0100, Nick Clifton wrote:
> 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:

 I think it could be confusing to have 0 and whatever constant, but I
 see your point.  Note that other struct elements are actually used as
 the sentinal so it kind of doesn't matter what value they have.

> > +++ 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.

that makes sense, reducing the amount of static data seems slightly
nice, and I kind of feel like a compiler can optimize that better.

> > +++ 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.

yeah, I mostly wrote that hunk as a this seems wierd let me see what
somebody who actually knows this thinks should be happening thing.

Trev