diff mbox

implement support for "enum class"

Message ID 1396005664.3824.70.camel@bordewijk.wildebeest.org
State Committed
Headers show

Commit Message

Mark Wielaard March 28, 2014, 11:21 a.m. UTC
On Thu, 2014-03-27 at 14:11 -0600, Tom Tromey wrote:
> This adds support for the C++11 "enum class" feature.  This is
> PR c++/15246.
> 
> I chose to use the existing TYPE_DECLARED_CLASS rather than introduce
> a new type code.  This seemed both simple and clear to me.
> 
> I made overloading support for the new enum types strict.  This is how
> it works in C++; and it didn't seem like an undue burden to keep this,
> particularly because enum constants are printed symbolically by gdb.
> 
> Built and regtested on x86-64 Fedora 20.
> [...]
> +gdb_test "ptype E1" \
> +    "type = enum class E1 {E1::HI = 7, E1::THERE}"

This FAILs for me with gcc plus
http://gcc.gnu.org/ml/gcc-patches/2014-03/msg01448.html
because it will derive:

Breakpoint 1, main () at /home/mark/src/binutils-gdb/gdb/testsuite/gdb.cp/enumclass.cc:45
45        return 0;
(gdb) ptype E1
type = enum class E1 : int {E1::HI = 7, E1::THERE}

Note the : int.

I think this comes from:

Cheers,

Mark

Comments

Tom Tromey March 31, 2014, 4:38 a.m. UTC | #1
>>>>> "Mark" == Mark Wielaard <mjw@redhat.com> writes:

>> +gdb_test "ptype E1" \
>> +    "type = enum class E1 {E1::HI = 7, E1::THERE}"

Mark> This FAILs for me with gcc plus
Mark> http://gcc.gnu.org/ml/gcc-patches/2014-03/msg01448.html

Thanks Mark.

Mark> (gdb) ptype E1
Mark> type = enum class E1 : int {E1::HI = 7, E1::THERE}
Mark> Note the : int.
[...]
Mark> If the test is right then I think you don't want to print the underlying
Mark> type if DW_AT_enum_class is set and the enum has type safe semantics?

I'm not totally sure what to do.  It seems that without extra
information there is no perfect answer.

At least this code should (most likely) only print the ": type" part for
C++, not for C.  That was an oversight in the original patch.  This may
omit some information from the C case, but on the other hand at least
the output won't be gratuitously non-C.

I was anticipating that GCC would emit an unnamed type for the
underlying type in the case where the underlying type is not specified
in the source.  However I can see why it would not necessarily do this,
and certainly there's no requirement in DWARF along these lines.

One option might be to have the DWARF reader omit the underlying type
when the type it guessed from the enum values agreed with the type
specified in DW_AT_type.  Of course that's just a heuristic.  It's
perhaps better, I think, to just always emit the bits even if they are
in some sense redundant.

In C++11, whether an underlying type is specified is orthogonal to
whether the enum is an "enum class".  "enum x : short" is perfectly ok.

Tom
Mark Wielaard March 31, 2014, 7:16 a.m. UTC | #2
Hi Tom,

On Sun, 2014-03-30 at 22:38 -0600, Tom Tromey wrote:
> >>>>> "Mark" == Mark Wielaard <mjw@redhat.com> writes:
> Mark> If the test is right then I think you don't want to print the underlying
> Mark> type if DW_AT_enum_class is set and the enum has type safe semantics?
> 
> I'm not totally sure what to do.  It seems that without extra
> information there is no perfect answer.
> 
> At least this code should (most likely) only print the ": type" part for
> C++, not for C.  That was an oversight in the original patch.  This may
> omit some information from the C case, but on the other hand at least
> the output won't be gratuitously non-C.

Yes, that seems to be the right way, if you want to print the type as
something that is a valid expression. Note that the underlying type in C
is almost always just "int", except when using -fshort-enums then GCC
uses the smallest representation like C++.

> I was anticipating that GCC would emit an unnamed type for the
> underlying type in the case where the underlying type is not specified
> in the source.  However I can see why it would not necessarily do this,
> and certainly there's no requirement in DWARF along these lines.

Funny. You pointed out my original patch accidentally did this
sometimes. Causing lots of extra base type DIEs. I indeed fixed this to
emit the already known base type in the CU (which often is the named int
base type) to reduce duplication. I think this is the correct thing to
do.

> One option might be to have the DWARF reader omit the underlying type
> when the type it guessed from the enum values agreed with the type
> specified in DW_AT_type.  Of course that's just a heuristic.  It's
> perhaps better, I think, to just always emit the bits even if they are
> in some sense redundant.
> 
> In C++11, whether an underlying type is specified is orthogonal to
> whether the enum is an "enum class".  "enum x : short" is perfectly ok.

Aha, you are right. And there is no way to know from the DWARF whether
the compiler derived the underlying type or the user explicitly defined
it.

I am also not sure what the right thing to do is here. But I think it
makes sense to make printing it depend on the language used and always
print it when the language supports "derived enums" whether the user
explicitly defined it or it was guessed by the compiler. It is an valid
expression in any case and the GDB user might appreciate the extra info
(in the case of C++).

Cheers,

Mark
diff mbox

Patch

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 4edc9ec..04b6d7b 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> [...]
> @@ -1349,6 +1351,14 @@ c_type_print_base (struct type *type, struct ui_file *stream,
>  	{
>  	  LONGEST lastval = 0;
>  
> +	  if (TYPE_TARGET_TYPE (type) != NULL)
> +	    {
> +	      struct type *underlying = check_typedef (TYPE_TARGET_TYPE (type));
> +
> +	      if (TYPE_NAME (underlying) != NULL)
> +		fprintf_filtered (stream, ": %s ", TYPE_NAME (underlying));
> +	    }
> +
>  	  fprintf_filtered (stream, "{");
>  	  len = TYPE_NFIELDS (type);
>  	  for (i = 0; i < len; i++)

If the test is right then I think you don't want to print the underlying
type if DW_AT_enum_class is set and the enum has type safe semantics?
Maybe:

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 04b6d7b..299d859 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -1351,7 +1351,7 @@  c_type_print_base (struct type *type, struct ui_file *stream,
        {
          LONGEST lastval = 0;
 
-         if (TYPE_TARGET_TYPE (type) != NULL)
+         if (TYPE_TARGET_TYPE (type) != NULL && ! TYPE_DECLARED_CLASS (type))
            {
              struct type *underlying = check_typedef (TYPE_TARGET_TYPE (type));