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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2016-01-20 13:03, Simon Marchi wrote:
> On 2016-01-20 09:41, Pedro Alves wrote:
>> 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
> 
> Here is a v2:
> 
> 
> From 5d7a3227fa50594c1f5541550a07481583e027df Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Mon, 18 Jan 2016 21:35:18 -0500
> Subject: [PATCH] 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
> 
> New in v2:
> 
> - Improved test case, I stole Pedro's example directly.  It verifies
>   that the sorting of enumerators by value works, by checking that
>   printing FOO_MASK appears as FOO_1 | FOO_2 | FOO_3.
> 
>   I noticed that I could change the regexps to almost anything and the
>   tests would still pass.  I think it was because of the | in there.  I
>   made them more robust by using string_to_regexp.  I used curly braces
>   { } instead of quoting marks " " for strings, so that I could use
>   square brackets [ ] in them without having to escape them all.  I 
> also
>   removed the "message" part of the tests, since they are redundant 
> with
>   the command, and it's just more maintenance to have to update them.
> 
>   Tested with Python 2.7 and 3.5.
> 
> gdb/ChangeLog:
> 
> 	* python/lib/gdb/printing.py (FlagEnumerationPrinter.__call__):
> 	Fix enumerators sort key function.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/py-pp-maint.exp: Change/add enum flag tests.
> 	* gdb.python/py-pp-maint.c (enum flag_enum): Use more complex
> 	enum flag values.
> ---
>  gdb/python/lib/gdb/printing.py           |  2 +-
>  gdb/testsuite/gdb.python/py-pp-maint.c   | 16 ++++++++++++----
>  gdb/testsuite/gdb.python/py-pp-maint.exp | 27 
> ++++++++++++++++++---------
>  3 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/python/lib/gdb/printing.py 
> b/gdb/python/lib/gdb/printing.py
> index 5160581..63c3aeb 100644
> --- a/gdb/python/lib/gdb/printing.py
> +++ b/gdb/python/lib/gdb/printing.py
> @@ -263,7 +263,7 @@ class FlagEnumerationPrinter(PrettyPrinter):
>                  self.enumerators.append((field.name, field.enumval))
>              # Sorting the enumerators by value usually does the right
>              # thing.
> -            self.enumerators.sort(key = lambda x: x.enumval)
> +            self.enumerators.sort(key = lambda x: x[1])
> 
>          if self.enabled:
>              return _EnumInstance(self.enumerators, val)
> diff --git a/gdb/testsuite/gdb.python/py-pp-maint.c
> b/gdb/testsuite/gdb.python/py-pp-maint.c
> index 657dfd7..d750496 100644
> --- a/gdb/testsuite/gdb.python/py-pp-maint.c
> +++ b/gdb/testsuite/gdb.python/py-pp-maint.c
> @@ -17,12 +17,20 @@
> 
>  #include <string.h>
> 
> +
>  enum flag_enum
>    {
> -    FLAG_1 = 1,
> -    FLAG_2 = 2,
> -    FLAG_3 = 4,
> -    ALL = FLAG_1 | FLAG_2 | FLAG_3
> +    /* Define the enumeration values in an unsorted manner to verify 
> that we
> +       effectively sort them by value.  */
> +    FOO_MASK = 0x07,
> +    FOO_1    = 0x01,
> +    FOO_2    = 0x02,
> +    FOO_3    = 0x04,
> +
> +    BAR_MASK = 0x70,
> +    BAR_1    = 0x10,
> +    BAR_2    = 0x20,
> +    BAR_3    = 0x40,
>    };
> 
>  enum flag_enum fval;
> diff --git a/gdb/testsuite/gdb.python/py-pp-maint.exp
> b/gdb/testsuite/gdb.python/py-pp-maint.exp
> index db0768f..9dbe19f 100644
> --- a/gdb/testsuite/gdb.python/py-pp-maint.exp
> +++ b/gdb/testsuite/gdb.python/py-pp-maint.exp
> @@ -119,14 +119,23 @@ gdb_test "print flt" " = x=<42> y=<43>" \
>  gdb_test "print ss" " = a=<a=<1> b=<$hex>> b=<a=<2> b=<$hex>>" \
>      "print ss re-enabled"
> 
> -gdb_test "print (enum flag_enum) (FLAG_1)" \
> -    " = 0x1 .FLAG_1." \
> -    "print FLAG_1"
> +gdb_test "print (enum flag_enum) (FOO_1)" \
> +    [string_to_regexp { = 0x1 [FOO_1]}]
> 
> -gdb_test "print (enum flag_enum) (FLAG_1 | FLAG_3)" \
> -    " = 0x5 .FLAG_1 | FLAG_3." \
> -    "print FLAG_1 | FLAG_3"
> +gdb_test "print (enum flag_enum) (BAR_3)" \
> +    [string_to_regexp { = 0x40 [BAR_3]}]
> 
> -gdb_test "print (enum flag_enum) (4 + 8)" \
> -    " = 0xc .FLAG_1 | <unknown: 0x8>." \
> -    "print FLAG_1 | 8"
> +gdb_test "print (enum flag_enum) (BAR_2 | FOO_2)" \
> +    [string_to_regexp { = 0x22 [FOO_2 | BAR_2]}]
> +
> +gdb_test "print (enum flag_enum) (FOO_1 | FOO_2 | FOO_3)" \
> +    [string_to_regexp { = 0x7 [FOO_1 | FOO_2 | FOO_3]}]
> +
> +gdb_test "print (enum flag_enum) (FOO_MASK)" \
> +    [string_to_regexp { = 0x7 [FOO_1 | FOO_2 | FOO_3]}]
> +
> +gdb_test "print (enum flag_enum) (FOO_MASK | (BAR_MASK & ~BAR_2))" \
> +    [string_to_regexp { = 0x57 [FOO_1 | FOO_2 | FOO_3 | BAR_1 | 
> BAR_3]}]
> +
> +gdb_test "print (enum flag_enum) (0x4 + 0x8)" \
> +    [string_to_regexp { = 0xc [FOO_3 | <unknown: 0x8>]}]

I tried to apply my patch from here, and it says it's corrupt (I was 
using a web mail client).  Please look at this version sent with git 
send-email instead.

https://sourceware.org/ml/gdb-patches/2016-01/msg00485.html

Thanks,

Simon