Pretty print of C++11 scoped enums - request help towards a proper fix

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Pretty print of C++11 scoped enums - request help towards a proper fix

will wray
Re: "Pretty print of enumerator never prints the id,
     always falls back to C-style cast output"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364

The bug report gives a one-line 'fix' to enable output of enum id
but, for C++11 scoped enums, it fails to qualify as enum type::id.

The code is located in c-pretty-print.c
It has not been updated to deal with C++11 scoped enumerations.

'Separation of responsibilities' between c and cxx-pretty-print
seems fairly lax - it's convenient to push some c++ printing to c
(there are a few comments like /* This C++ bit is handled here...*/)

I have not quite managed to make a fix confined to c-pretty-print.c

I have a fix which duplicates the code in pp_c_enumeration_constant
to pp_cxx_enumeration_constant in cxx-pretty print, with modification

  if (value != NULL_TREE)
  {
    if (ENUM_IS_SCOPED (type))
      pp_cxx_nested_name_specifier (pp, type);
    pp->id_expression (TREE_PURPOSE (value));
  }

This works in my testing so far, but
 - It duplicates code from c to cxx (not DRY 'Don't Repeat Yourself)
 - I didn't find a single function to print full nested, scoped id
   so had to check if ENUM_IS_SCOPED to output nested specifiers.

I'm learning by hacking but would like guidance on a proper fix
from anyone more familiar with gcc pretty print and/or grammar -
the guideline comment, at the top of the file, states:

/* The pretty-printer code is primarily designed to closely follow
   (GNU) C and C++ grammars... */

I'd appreciate any recommendations towards a proper fix,
or pointers for how to write unit tests for the fix.

Thanks, Will
Reply | Threaded
Open this post in threaded view
|

Re: Pretty print of C++11 scoped enums - request help towards a proper fix

Nathan Sidwell-2
On 9/19/18 7:41 AM, will wray wrote:
> Re: "Pretty print of enumerator never prints the id,
>       always falls back to C-style cast output"
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364
>

> I have a fix which duplicates the code in pp_c_enumeration_constant
> to pp_cxx_enumeration_constant in cxx-pretty print, with modification
>
>    if (value != NULL_TREE)
>    {
>      if (ENUM_IS_SCOPED (type))
>        pp_cxx_nested_name_specifier (pp, type);
>      pp->id_expression (TREE_PURPOSE (value));
>    }
>
> This works in my testing so far, but
>   - It duplicates code from c to cxx (not DRY 'Don't Repeat Yourself)
>   - I didn't find a single function to print full nested, scoped id
>     so had to check if ENUM_IS_SCOPED to output nested specifiers.

This seems a fine approach, given the code base.

nathan

--
Nathan Sidwell
Reply | Threaded
Open this post in threaded view
|

Re: Pretty print of C++11 scoped enums - request help towards a proper fix

will wray
Thanks Nathan,


In fact, after testing with enums nested in namespaces or structs,

or function local, I realised nested specifiers should be printed

for both scoped and unscoped enums, but for unscoped enums one

level of nested specifier (the enum type) needs to be stripped.

So I inverted the IS_SCOPED test and used get_containing_scope:


if (value != NULL_TREE)
{

if (!ENUM_IS_SCOPED (type))

type = get_containing_scope (type);

pp_cxx_nested_name_specifier (pp, type);

pp->id_expression (TREE_PURPOSE (value));

}


I submitted this fix as a patch to the bug report, with tests.

With this fix GCC now has similar output to both Clang and MSVC
for enumerated values. For non-enumerated values GCC continues
to print a C-style cast while Clang & MSVC print plain digits.
Yay! GCC is winning! (gives type info for non-enumerated values).

A downside of nested specifiers is that output gets verbose.

Richard Smith suggests to use less verbose output for known types
compared to auto deduced types. Discussion starts here
http://lists.llvm.org/pipermail/cfe-dev/2018-September/059229.html

For enum args, I guess that this would mean distinguishing whether
the corresponding template parameter was auto or a given enum type
and only printing a simple id with no nested specs for given type.
I don't know yet if that info is available at the leaf level here.

Similarly, type info should be added to deduced Integral values.
I may start to investigate how to do this in GCC pretty print.
I submitted the related request to MSVC devs:
https://developercommunity.visualstudio.com/content/problem/339663/improve-pretty-print-of-integral-non-type-template.html

> given the code base ...

GCC pretty-print code was committed by GDR mid 2002,
K&R style C, updated to C90 'prototype' in mid 2003,
untouched since then, not for C++11 or C++17 enum updates.

I found this corner of the code base fairly easy to hack,
thanks perhaps to GDRs attempts to follow the grammar.


On Mon, Sep 24, 2018 at 3:53 PM Nathan Sidwell <[hidden email]> wrote:

> On 9/19/18 7:41 AM, will wray wrote:
> > Re: "Pretty print of enumerator never prints the id,
> >       always falls back to C-style cast output"
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364
> >
>
> > I have a fix which duplicates the code in pp_c_enumeration_constant
> > to pp_cxx_enumeration_constant in cxx-pretty print, with modification
> >
> >    if (value != NULL_TREE)
> >    {
> >      if (ENUM_IS_SCOPED (type))
> >        pp_cxx_nested_name_specifier (pp, type);
> >      pp->id_expression (TREE_PURPOSE (value));
> >    }
> >
> > This works in my testing so far, but
> >   - It duplicates code from c to cxx (not DRY 'Don't Repeat Yourself)
> >   - I didn't find a single function to print full nested, scoped id
> >     so had to check if ENUM_IS_SCOPED to output nested specifiers.
>
> This seems a fine approach, given the code base.
>
> nathan
>
> --
> Nathan Sidwell
>
Reply | Threaded
Open this post in threaded view
|

Re: Pretty print of C++11 scoped enums - request help towards a proper fix

will wray
BTW The bug is still UNCONFIRMED
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364

It is easy to CONFIRM:

Follow this Compiler Explorer link https://godbolt.org/z/P4ejiy
or paste this code into a file and compile with g++:

template <auto> struct wauto;
enum e { a };
wauto<a> v;    // error


Note that GCC reports  error: ... 'wauto<e(0)> v'
 => It should report   error: ... 'wauto<a> v'

This is a bug; the intent of the code is print the enumerator id
(clang prints the enumerator id and so do recent MSVC previews).
There is also test code linked to the bug covering more cases.

I'd appreciate if someone would confirm the bug.

Thanks, Will

On Mon, Sep 24, 2018 at 5:23 PM will wray <[hidden email]> wrote:

> Thanks Nathan,
>
>
> In fact, after testing with enums nested in namespaces or structs,
>
> or function local, I realised nested specifiers should be printed
>
> for both scoped and unscoped enums, but for unscoped enums one
>
> level of nested specifier (the enum type) needs to be stripped.
>
> So I inverted the IS_SCOPED test and used get_containing_scope:
>
>
> if (value != NULL_TREE)
> {
>
> if (!ENUM_IS_SCOPED (type))
>
> type = get_containing_scope (type);
>
> pp_cxx_nested_name_specifier (pp, type);
>
> pp->id_expression (TREE_PURPOSE (value));
>
> }
>
>
> I submitted this fix as a patch to the bug report, with tests.
>
> With this fix GCC now has similar output to both Clang and MSVC
> for enumerated values. For non-enumerated values GCC continues
> to print a C-style cast while Clang & MSVC print plain digits.
> Yay! GCC is winning! (gives type info for non-enumerated values).
>
> A downside of nested specifiers is that output gets verbose.
>
> Richard Smith suggests to use less verbose output for known types
> compared to auto deduced types. Discussion starts here
> http://lists.llvm.org/pipermail/cfe-dev/2018-September/059229.html
>
> For enum args, I guess that this would mean distinguishing whether
> the corresponding template parameter was auto or a given enum type
> and only printing a simple id with no nested specs for given type.
> I don't know yet if that info is available at the leaf level here.
>
> Similarly, type info should be added to deduced Integral values.
> I may start to investigate how to do this in GCC pretty print.
> I submitted the related request to MSVC devs:
>
> https://developercommunity.visualstudio.com/content/problem/339663/improve-pretty-print-of-integral-non-type-template.html
>
> > given the code base ...
>
> GCC pretty-print code was committed by GDR mid 2002,
> K&R style C, updated to C90 'prototype' in mid 2003,
> untouched since then, not for C++11 or C++17 enum updates.
>
> I found this corner of the code base fairly easy to hack,
> thanks perhaps to GDRs attempts to follow the grammar.
>
>
> On Mon, Sep 24, 2018 at 3:53 PM Nathan Sidwell <[hidden email]> wrote:
>
>> On 9/19/18 7:41 AM, will wray wrote:
>> > Re: "Pretty print of enumerator never prints the id,
>> >       always falls back to C-style cast output"
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364
>> >
>>
>> > I have a fix which duplicates the code in pp_c_enumeration_constant
>> > to pp_cxx_enumeration_constant in cxx-pretty print, with modification
>> >
>> >    if (value != NULL_TREE)
>> >    {
>> >      if (ENUM_IS_SCOPED (type))
>> >        pp_cxx_nested_name_specifier (pp, type);
>> >      pp->id_expression (TREE_PURPOSE (value));
>> >    }
>> >
>> > This works in my testing so far, but
>> >   - It duplicates code from c to cxx (not DRY 'Don't Repeat Yourself)
>> >   - I didn't find a single function to print full nested, scoped id
>> >     so had to check if ENUM_IS_SCOPED to output nested specifiers.
>>
>> This seems a fine approach, given the code base.
>>
>> nathan
>>
>> --
>> Nathan Sidwell
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Pretty print of C++11 scoped enums - request help towards a proper fix

will wray
Patch submitted:

https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00452.html
[C++ PATCH] Fix pretty-print of enumerator ids (PR c++/87364)

My first GCC patch attempt, so more eyes would be good.

Cheers, Will

On Tue, Sep 25, 2018 at 4:25 PM will wray <[hidden email]> wrote:

> BTW The bug is still UNCONFIRMED
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364
>
> It is easy to CONFIRM:
>
> Follow this Compiler Explorer link https://godbolt.org/z/P4ejiy
> or paste this code into a file and compile with g++:
>
> template <auto> struct wauto;
> enum e { a };
> wauto<a> v;    // error
>
>
> Note that GCC reports  error: ... 'wauto<e(0)> v'
>  => It should report   error: ... 'wauto<a> v'
>
> This is a bug; the intent of the code is print the enumerator id
> (clang prints the enumerator id and so do recent MSVC previews).
> There is also test code linked to the bug covering more cases.
>
> I'd appreciate if someone would confirm the bug.
>
> Thanks, Will
>
> On Mon, Sep 24, 2018 at 5:23 PM will wray <[hidden email]> wrote:
>
>> Thanks Nathan,
>>
>>
>> In fact, after testing with enums nested in namespaces or structs,
>>
>> or function local, I realised nested specifiers should be printed
>>
>> for both scoped and unscoped enums, but for unscoped enums one
>>
>> level of nested specifier (the enum type) needs to be stripped.
>>
>> So I inverted the IS_SCOPED test and used get_containing_scope:
>>
>>
>> if (value != NULL_TREE)
>> {
>>
>> if (!ENUM_IS_SCOPED (type))
>>
>> type = get_containing_scope (type);
>>
>> pp_cxx_nested_name_specifier (pp, type);
>>
>> pp->id_expression (TREE_PURPOSE (value));
>>
>> }
>>
>>
>> I submitted this fix as a patch to the bug report, with tests.
>>
>> With this fix GCC now has similar output to both Clang and MSVC
>> for enumerated values. For non-enumerated values GCC continues
>> to print a C-style cast while Clang & MSVC print plain digits.
>> Yay! GCC is winning! (gives type info for non-enumerated values).
>>
>> A downside of nested specifiers is that output gets verbose.
>>
>> Richard Smith suggests to use less verbose output for known types
>> compared to auto deduced types. Discussion starts here
>> http://lists.llvm.org/pipermail/cfe-dev/2018-September/059229.html
>>
>> For enum args, I guess that this would mean distinguishing whether
>> the corresponding template parameter was auto or a given enum type
>> and only printing a simple id with no nested specs for given type.
>> I don't know yet if that info is available at the leaf level here.
>>
>> Similarly, type info should be added to deduced Integral values.
>> I may start to investigate how to do this in GCC pretty print.
>> I submitted the related request to MSVC devs:
>>
>> https://developercommunity.visualstudio.com/content/problem/339663/improve-pretty-print-of-integral-non-type-template.html
>>
>> > given the code base ...
>>
>> GCC pretty-print code was committed by GDR mid 2002,
>> K&R style C, updated to C90 'prototype' in mid 2003,
>> untouched since then, not for C++11 or C++17 enum updates.
>>
>> I found this corner of the code base fairly easy to hack,
>> thanks perhaps to GDRs attempts to follow the grammar.
>>
>>
>> On Mon, Sep 24, 2018 at 3:53 PM Nathan Sidwell <[hidden email]> wrote:
>>
>>> On 9/19/18 7:41 AM, will wray wrote:
>>> > Re: "Pretty print of enumerator never prints the id,
>>> >       always falls back to C-style cast output"
>>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87364
>>> >
>>>
>>> > I have a fix which duplicates the code in pp_c_enumeration_constant
>>> > to pp_cxx_enumeration_constant in cxx-pretty print, with modification
>>> >
>>> >    if (value != NULL_TREE)
>>> >    {
>>> >      if (ENUM_IS_SCOPED (type))
>>> >        pp_cxx_nested_name_specifier (pp, type);
>>> >      pp->id_expression (TREE_PURPOSE (value));
>>> >    }
>>> >
>>> > This works in my testing so far, but
>>> >   - It duplicates code from c to cxx (not DRY 'Don't Repeat Yourself)
>>> >   - I didn't find a single function to print full nested, scoped id
>>> >     so had to check if ENUM_IS_SCOPED to output nested specifiers.
>>>
>>> This seems a fine approach, given the code base.
>>>
>>> nathan
>>>
>>> --
>>> Nathan Sidwell
>>>
>>