[1/2] Fix sorting of enum values in FlagEnumerationPrinter

  The lambda function used to sort the enumerator list does not work
properly.  This list consists of tuples, (enum label, enum value).  The
key function returns x.enumval.  enumval not being defined for a tuple,
we see this exception in the test log:

  Python Exception <class 'AttributeError'> 'tuple' object has no attribute 'enumval'

The function should return the second item of the tuple, which is the
enumval.

The pretty-printer still worked mostly correctly, except that the
enumeration values were not sorted.  The test still passed because the
enumeration values are already sorted where they are defined.  The test
also passed despite the exception being printed, because the right output
was printed after the exception:

  print (enum flag_enum) (FLAG_1)
  Python Exception <type 'exceptions.AttributeError'> 'tuple' objecthas no attribute 'enumval':M
  $7 = 0x1 [FLAG_1]
  (gdb) PASS: gdb.python/py-pp-maint.exp: print FLAG_1

To properly test the sorting, I changed the order in which the
enumeration values are defined.

gdb/ChangeLog:

	* python/lib/gdb/printing.py (FlagEnumerationPrinter.__call__):
	Fix enumerators sort key function.

gdb/testsuite/ChangeLog:

	* gdb.python/py-pp-main.c (enum flag_enum): Change enum values
	definition order.
---
 gdb/python/lib/gdb/printing.py         | 2 +-
 gdb/testsuite/gdb.python/py-pp-maint.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)
  

Thanks for catching this.

I find it surprising that the printer doesn't respect the
order of the values as they're defined though.  Shouldn't we
remove the sort line entirely, thus keeping the
existing behavior?  I couldn't find mention of the sorting
in the documentation either.

Or, maybe the printer doesn't work correctly if the "overlapping"
value (which I think it the whole point of this printer) is defined
before the particular values, like, e.g.:

 enum flag_enum
   {
     ALL = 1 | 2 | 4,
     FLAG_2 = 2,
     FLAG_3 = 4,
     FLAG_1 = 1,
   };

?

On 01/19/2016 04:23 AM, Simon Marchi wrote:

> +
>  enum flag_enum
>    {
> -    FLAG_1 = 1,
> +    /* Define the enumration values in an unsorted manner to verify that we
> +       effectively sort them by value.  */

typo: "enumration".

Thanks,
Pedro Alves
  

On 2016-01-19 06:02, Pedro Alves wrote:
> Thanks for catching this.
> 
> I find it surprising that the printer doesn't respect the
> order of the values as they're defined though.  Shouldn't we
> remove the sort line entirely, thus keeping the
> existing behavior?  I couldn't find mention of the sorting
> in the documentation either.
> 
> Or, maybe the printer doesn't work correctly if the "overlapping"
> value (which I think it the whole point of this printer) is defined
> before the particular values, like, e.g.:
> 
>  enum flag_enum
>    {
>      ALL = 1 | 2 | 4,
>      FLAG_2 = 2,
>      FLAG_3 = 4,
>      FLAG_1 = 1,
>    };
> 
> ?

If we don't sort the values and ALL is defined first, then 0x7 will be
displayed as ALL instead of FLAG_1 | FLAG_2 | FLAG_3.  I don't think
either is wrong, we just don't know which one each particular user
would prefer.  So I think we can choose one way (sorted order, or
definition order) and stick with it.

Personally, I think I would prefer the more explicit version
(FLAG_1 | FLAG_2 | FLAG_3), which means keeping the sort.

> On 01/19/2016 04:23 AM, Simon Marchi wrote:
> 
>> +
>>  enum flag_enum
>>    {
>> -    FLAG_1 = 1,
>> +    /* Define the enumration values in an unsorted manner to verify 
>> that we
>> +       effectively sort them by value.  */
> 
> typo: "enumration".

Fixed.
  

On 01/19/2016 04:41 PM, Simon Marchi wrote:
> On 2016-01-19 06:02, Pedro Alves wrote:
>> Thanks for catching this.
>>
>> I find it surprising that the printer doesn't respect the
>> order of the values as they're defined though.  Shouldn't we
>> remove the sort line entirely, thus keeping the
>> existing behavior?  I couldn't find mention of the sorting
>> in the documentation either.
>>
>> Or, maybe the printer doesn't work correctly if the "overlapping"
>> value (which I think it the whole point of this printer) is defined
>> before the particular values, like, e.g.:
>>
>>  enum flag_enum
>>    {
>>      ALL = 1 | 2 | 4,
>>      FLAG_2 = 2,
>>      FLAG_3 = 4,
>>      FLAG_1 = 1,
>>    };
>>
>> ?
> 
> If we don't sort the values and ALL is defined first, then 0x7 will be
> displayed as ALL instead of FLAG_1 | FLAG_2 | FLAG_3.  I don't think
> either is wrong, we just don't know which one each particular user
> would prefer.  So I think we can choose one way (sorted order, or
> definition order) and stick with it.
> 
> Personally, I think I would prefer the more explicit version
> (FLAG_1 | FLAG_2 | FLAG_3), which means keeping the sort.

OK, I think that makes sense for cases like:

  enum flag_enum
   {
     FOO_MASK = 0x07,
     FOO_1    = 0x01,
     FOO_2    = 0x02,
     FOO_3    = 0x04,

     BAR_MASK = 0x70,
     BAR_1    = 0x10,
     BAR_2    = 0x20,
     BAR_3    = x040,
   };

Would you mind augmenting the testsuite with something
like this, then?

Thanks,
Pedro Alves