[(for,gdb)] enum-flags.h: Add trailing semicolon to example in comment

Message ID 1528221387-44895-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • [(for,gdb)] enum-flags.h: Add trailing semicolon to example in comment
Related show

Commit Message

David Malcolm June 5, 2018, 5:56 p.m.
On Tue, 2018-06-05 at 17:13 +0100, Pedro Alves wrote:
> On 06/05/2018 03:49 PM, David Malcolm wrote:

> > On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote:

> > > You may want to look at gdb's enum-flags.h which I think already

> > > implements what your doing here.

> > 

> > Aha!  Thanks!

> > 

> > Browsing the git web UI, that gdb header was introduced by Pedro

> > Alves

> > in:

> >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit

> > diff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9

> > and the current state is here:

> >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f

> > =gdb/common/enum-flags.h;hb=HEAD

> > 

> > I'll try this out with GCC; hopefully it works with C++98.

> 

> It should -- it was written/added while GDB still required C++98.


Thanks.

> Note I have a WIP series here:

> 

>  https://github.com/palves/gdb/commits/palves/cxx11-enum-flags

> 

> that fixes a few things, and adds a bunch of unit tests.  In

> the process, it uses C++11 constructs (hence the branch's name),

> but I think that can be reverted to still work with C++98.

> 

> I had seen some noises recently about GCC maybe considering

> requiring C++11.  Is that reasonable to expect in this cycle?

> (GDB requires GCC 4.8 or later, FWIW.)


I'm expecting that GCC 9 will still require C++98.

> Getting that branch into master had fallen lower on my TODO,

> but if there's interest, I can try to finish it off soon, so we

> can share a better baseline.  (I posted it once, but found

> some issues which I fixed since but never managed to repost.)


Interestingly, it looks like gdb is using Google Test for selftests;
gcc is using a hand-rolled API that is similar, but has numerous
differences (e.g. explicit calling of test functions, rather than
implicit test discovery).  So that's another area of drift between
the projects.

> > 

> > Presumably it would be good to share this header between GCC and

> > GDB;

> > CCing Pedro; Pedro: hi!  Does this sound sane?

> > (for reference, the GCC patch we're discussing here is:

> >   https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html )

> 

> Sure!


Alternatively, if GCC needs to stay at C++98 and GDB moves on to C++11,
then we can at least take advantage of this tested and FSF-assigned
C++98 code (better than writing it from scratch).

I ran into one issue with the header, e.g. with:

  /* Dump flags type.  */
  DEF_ENUM_FLAGS_TYPE(enum dump_flag, dump_flags_t);

This doesn't work:
  TDF_SLIM | flags
but this does:
  flags | TDF_SLIM
where TDF_SLIM is one of the values of "enum dump_flag".

For example, the former leads to:

../../src/gcc/c-family/c-gimplify.c: In function ‘void c_genericize(tree)’:
../../src/gcc/c-family/c-gimplify.c:145:44: error: conversion from ‘int’ to ‘dump_flags_t’ {aka ‘enum_flags<dump_flag>’} is ambiguous
      TDF_SLIM | local_dump_flags, dump_orig);
                                            ^
In file included from ../../src/gcc/dumpfile.h:24,
                 from ../../src/gcc/coretypes.h:428,
                 from ../../src/gcc/c-family/c-gimplify.c:28:
../../src/gcc/../include/enum-flags.h:128:3: note: candidate: ‘enum_flags<E>::enum_flags(enum_flags<E>::zero_type*) [with E = dump_flag]’ <near match>
   enum_flags (struct enum_flags::zero_type *zero)
   ^~~~~~~~~~
../../src/gcc/../include/enum-flags.h:128:3: note:   conversion of argument 1 would be ill-formed:
../../src/gcc/c-family/c-gimplify.c:145:15: error: invalid conversion from ‘int’ to ‘enum_flags<dump_flag>::zero_type*’ [-fpermissive]
      TDF_SLIM | local_dump_flags, dump_orig);
      ~~~~~~~~~^~~~~~~~~~~~~~~~~~
In file included from ../../src/gcc/dumpfile.h:24,
                 from ../../src/gcc/coretypes.h:428,
                 from ../../src/gcc/c-family/c-gimplify.c:28:
../../src/gcc/../include/enum-flags.h:125:3: note: candidate: ‘enum_flags<E>::enum_flags(enum_flags<E>::enum_type) [with E = dump_flag; enum_flags<E>::enum_type = dump_flag]’ <near match>
   enum_flags (enum_type e)
   ^~~~~~~~~~
../../src/gcc/../include/enum-flags.h:125:3: note:   conversion of argument 1 would be ill-formed:
../../src/gcc/c-family/c-gimplify.c:145:15: error: invalid conversion from ‘int’ to ‘enum_flags<dump_flag>::enum_type’ {aka ‘dump_flag’} [-fpermissive]
      TDF_SLIM | local_dump_flags, dump_orig);
      ~~~~~~~~~^~~~~~~~~~~~~~~~~~
In file included from ../../src/gcc/coretypes.h:428,
                 from ../../src/gcc/c-family/c-gimplify.c:28:
../../src/gcc/dumpfile.h:248:13: note:   initializing argument 2 of ‘void dump_node(const_tree, dump_flags_t, FILE*)’
 extern void dump_node (const_tree, dump_flags_t, FILE *);
             ^~~~~~~~~

I'm not quite sure why it doesn't work that way around, but reversing
the order so that the dump_flags_t is on the left-hand-side lets it
work (it only affects 3 sites in gcc's source tree).


> Thanks,

> Pedro Alves


Thanks.

BTW, I spotted this trivial issue in a comment in enum-flags.h whilst
trying it out for gcc:



The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing
semicolon, but the example in the comment lacks one.

	* enum-flags.h: Add trailing semicolon to example in comment.
---
 include/enum-flags.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.5.3

Comments

Pedro Alves June 5, 2018, 5:31 p.m. | #1
[adding gdb-patches]

On 06/05/2018 06:56 PM, David Malcolm wrote:
> On Tue, 2018-06-05 at 17:13 +0100, Pedro Alves wrote:

>> On 06/05/2018 03:49 PM, David Malcolm wrote:

>>> On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote:

>>>> You may want to look at gdb's enum-flags.h which I think already

>>>> implements what your doing here.

>>>

>>> Aha!  Thanks!

>>>

>>> Browsing the git web UI, that gdb header was introduced by Pedro

>>> Alves

>>> in:

>>>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit

>>> diff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9

>>> and the current state is here:

>>>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f

>>> =gdb/common/enum-flags.h;hb=HEAD

>>>

>>> I'll try this out with GCC; hopefully it works with C++98.

>>

>> It should -- it was written/added while GDB still required C++98.

> 

> Thanks.

> 

>> Note I have a WIP series here:

>>

>>  https://github.com/palves/gdb/commits/palves/cxx11-enum-flags

>>

>> that fixes a few things, and adds a bunch of unit tests.  In

>> the process, it uses C++11 constructs (hence the branch's name),

>> but I think that can be reverted to still work with C++98.

>>

>> I had seen some noises recently about GCC maybe considering

>> requiring C++11.  Is that reasonable to expect in this cycle?

>> (GDB requires GCC 4.8 or later, FWIW.)

> 

> I'm expecting that GCC 9 will still require C++98.


OK.

> 

>> Getting that branch into master had fallen lower on my TODO,

>> but if there's interest, I can try to finish it off soon, so we

>> can share a better baseline.  (I posted it once, but found

>> some issues which I fixed since but never managed to repost.)

> 

> Interestingly, it looks like gdb is using Google Test for selftests;

> gcc is using a hand-rolled API that is similar, but has numerous

> differences (e.g. explicit calling of test functions, rather than

> implicit test discovery).  So that's another area of drift between

> the projects.


Nope, the unit tests framework is hand rolled too.  See gdb/selftest.h/c.
It can be invoked with gdb's "maint selftest" command.
You can see the list of tests with "maint info selftests", and
you can pass a filter to "maint selftest" too.

> 

>>>

>>> Presumably it would be good to share this header between GCC and

>>> GDB;

>>> CCing Pedro; Pedro: hi!  Does this sound sane?

>>> (for reference, the GCC patch we're discussing here is:

>>>   https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html )

>>

>> Sure!

> 

> Alternatively, if GCC needs to stay at C++98 and GDB moves on to C++11,

> then we can at least take advantage of this tested and FSF-assigned

> C++98 code (better than writing it from scratch).


Agreed, but I'll try to see about making the fixes in the branch
C++98 compatible.

> 

> I ran into one issue with the header, e.g. with:

> 

>   /* Dump flags type.  */

>   DEF_ENUM_FLAGS_TYPE(enum dump_flag, dump_flags_t);

> 

> This doesn't work:

>   TDF_SLIM | flags

> but this does:

>   flags | TDF_SLIM

> where TDF_SLIM is one of the values of "enum dump_flag".


ISTR that that's fixed in the branch.

> BTW, I spotted this trivial issue in a comment in enum-flags.h whilst

> trying it out for gcc:

> 

> 

> 

> The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing

> semicolon, but the example in the comment lacks one.

> 

> 	* enum-flags.h: Add trailing semicolon to example in comment.


Thanks.  I've merged it.

From 115f7325b5b76450b9a1d16848a2a54f54e49747 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>

Date: Tue, 5 Jun 2018 18:22:25 +0100
Subject: [PATCH] Fix typo in common/enum-flags.h example

The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing
semicolon, but the example in the comment lacks one.

gdb/ChangeLog:
2018-06-05  David Malcolm  <dmalcolm@redhat.com>

	* common/enum-flags.h: Add trailing semicolon to example in
	comment.
---
 gdb/ChangeLog           | 5 +++++
 gdb/common/enum-flags.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 85c62dbd86c..54205a6ea85 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-06-05  David Malcolm  <dmalcolm@redhat.com>
+
+	* common/enum-flags.h: Add trailing semicolon to example in
+	comment.
+
 2018-06-05  Tom Tromey	<tom@tromey.com>
 
 	PR cli/12326:
diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h
index 65732c11a46..82568a5a3d9 100644
--- a/gdb/common/enum-flags.h
+++ b/gdb/common/enum-flags.h
@@ -32,7 +32,7 @@
        flag_val3 = 1 << 3,
        flag_val4 = 1 << 4,
     };
-    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)
+    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags);
 
     some_flags f = flag_val1 | flag_val2;
     f |= flag_val3;
-- 
2.14.3

Thanks,
Pedro Alves
Eric Gallager June 6, 2018, 12:48 a.m. | #2
On 6/5/18, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2018-06-05 at 17:13 +0100, Pedro Alves wrote:

>> On 06/05/2018 03:49 PM, David Malcolm wrote:

>> > On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote:

>> > > You may want to look at gdb's enum-flags.h which I think already

>> > > implements what your doing here.

>> >

>> > Aha!  Thanks!

>> >

>> > Browsing the git web UI, that gdb header was introduced by Pedro

>> > Alves

>> > in:

>> >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit

>> > diff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9

>> > and the current state is here:

>> >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f

>> > =gdb/common/enum-flags.h;hb=HEAD

>> >

>> > I'll try this out with GCC; hopefully it works with C++98.

>>

>> It should -- it was written/added while GDB still required C++98.

>

> Thanks.

>

>> Note I have a WIP series here:

>>

>>  https://github.com/palves/gdb/commits/palves/cxx11-enum-flags

>>

>> that fixes a few things, and adds a bunch of unit tests.  In

>> the process, it uses C++11 constructs (hence the branch's name),

>> but I think that can be reverted to still work with C++98.

>>

>> I had seen some noises recently about GCC maybe considering

>> requiring C++11.  Is that reasonable to expect in this cycle?

>> (GDB requires GCC 4.8 or later, FWIW.)

>

> I'm expecting that GCC 9 will still require C++98.


Not that I particularly want GCC to move to C++11 myself, but could
GCC at least switch from building with -Wno-narrowing to building with
-Wnarrowing? That'd make it easier to switch to C++11 when the time
comes, if anyone actually wants to do that.

>

>> Getting that branch into master had fallen lower on my TODO,

>> but if there's interest, I can try to finish it off soon, so we

>> can share a better baseline.  (I posted it once, but found

>> some issues which I fixed since but never managed to repost.)

>

> Interestingly, it looks like gdb is using Google Test for selftests;

> gcc is using a hand-rolled API that is similar, but has numerous

> differences (e.g. explicit calling of test functions, rather than

> implicit test discovery).  So that's another area of drift between

> the projects.

>

>> >

>> > Presumably it would be good to share this header between GCC and

>> > GDB;

>> > CCing Pedro; Pedro: hi!  Does this sound sane?

>> > (for reference, the GCC patch we're discussing here is:

>> >   https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html )

>>

>> Sure!

>

> Alternatively, if GCC needs to stay at C++98 and GDB moves on to C++11,

> then we can at least take advantage of this tested and FSF-assigned

> C++98 code (better than writing it from scratch).

>

> I ran into one issue with the header, e.g. with:

>

>   /* Dump flags type.  */

>   DEF_ENUM_FLAGS_TYPE(enum dump_flag, dump_flags_t);

>

> This doesn't work:

>   TDF_SLIM | flags

> but this does:

>   flags | TDF_SLIM

> where TDF_SLIM is one of the values of "enum dump_flag".

>

> For example, the former leads to:

>

> ../../src/gcc/c-family/c-gimplify.c: In function ‘void c_genericize(tree)’:

> ../../src/gcc/c-family/c-gimplify.c:145:44: error: conversion from ‘int’ to

> ‘dump_flags_t’ {aka ‘enum_flags<dump_flag>’} is ambiguous

>       TDF_SLIM | local_dump_flags, dump_orig);

>                                             ^

> In file included from ../../src/gcc/dumpfile.h:24,

>                  from ../../src/gcc/coretypes.h:428,

>                  from ../../src/gcc/c-family/c-gimplify.c:28:

> ../../src/gcc/../include/enum-flags.h:128:3: note: candidate:

> ‘enum_flags<E>::enum_flags(enum_flags<E>::zero_type*) [with E = dump_flag]’

> <near match>

>    enum_flags (struct enum_flags::zero_type *zero)

>    ^~~~~~~~~~

> ../../src/gcc/../include/enum-flags.h:128:3: note:   conversion of argument

> 1 would be ill-formed:

> ../../src/gcc/c-family/c-gimplify.c:145:15: error: invalid conversion from

> ‘int’ to ‘enum_flags<dump_flag>::zero_type*’ [-fpermissive]

>       TDF_SLIM | local_dump_flags, dump_orig);

>       ~~~~~~~~~^~~~~~~~~~~~~~~~~~

> In file included from ../../src/gcc/dumpfile.h:24,

>                  from ../../src/gcc/coretypes.h:428,

>                  from ../../src/gcc/c-family/c-gimplify.c:28:

> ../../src/gcc/../include/enum-flags.h:125:3: note: candidate:

> ‘enum_flags<E>::enum_flags(enum_flags<E>::enum_type) [with E = dump_flag;

> enum_flags<E>::enum_type = dump_flag]’ <near match>

>    enum_flags (enum_type e)

>    ^~~~~~~~~~

> ../../src/gcc/../include/enum-flags.h:125:3: note:   conversion of argument

> 1 would be ill-formed:

> ../../src/gcc/c-family/c-gimplify.c:145:15: error: invalid conversion from

> ‘int’ to ‘enum_flags<dump_flag>::enum_type’ {aka ‘dump_flag’}

> [-fpermissive]

>       TDF_SLIM | local_dump_flags, dump_orig);

>       ~~~~~~~~~^~~~~~~~~~~~~~~~~~

> In file included from ../../src/gcc/coretypes.h:428,

>                  from ../../src/gcc/c-family/c-gimplify.c:28:

> ../../src/gcc/dumpfile.h:248:13: note:   initializing argument 2 of ‘void

> dump_node(const_tree, dump_flags_t, FILE*)’

>  extern void dump_node (const_tree, dump_flags_t, FILE *);

>              ^~~~~~~~~

>

> I'm not quite sure why it doesn't work that way around, but reversing

> the order so that the dump_flags_t is on the left-hand-side lets it

> work (it only affects 3 sites in gcc's source tree).

>

>

>> Thanks,

>> Pedro Alves

>

> Thanks.

>

> BTW, I spotted this trivial issue in a comment in enum-flags.h whilst

> trying it out for gcc:

>

>

>

> The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing

> semicolon, but the example in the comment lacks one.

>

> 	* enum-flags.h: Add trailing semicolon to example in comment.

> ---

>  include/enum-flags.h | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/include/enum-flags.h b/include/enum-flags.h

> index 65732c1..82568a5 100644

> --- a/include/enum-flags.h

> +++ b/include/enum-flags.h

> @@ -32,7 +32,7 @@

>         flag_val3 = 1 << 3,

>         flag_val4 = 1 << 4,

>      };

> -    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)

> +    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags);

>

>      some_flags f = flag_val1 | flag_val2;

>      f |= flag_val3;

> --

> 1.8.5.3

>

>
David Malcolm June 6, 2018, 12:54 p.m. | #3
On Tue, 2018-06-05 at 18:31 +0100, Pedro Alves wrote:
> [adding gdb-patches]

> 

> On 06/05/2018 06:56 PM, David Malcolm wrote:

> > On Tue, 2018-06-05 at 17:13 +0100, Pedro Alves wrote:

> > > On 06/05/2018 03:49 PM, David Malcolm wrote:

> > > > On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote:

> > > > > You may want to look at gdb's enum-flags.h which I think

> > > > > already

> > > > > implements what your doing here.

> > > > 

> > > > Aha!  Thanks!

> > > > 

> > > > Browsing the git web UI, that gdb header was introduced by

> > > > Pedro

> > > > Alves

> > > > in:

> > > >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=co

> > > > mmit

> > > > diff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9

> > > > and the current state is here:

> > > >   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=bl

> > > > ob;f

> > > > =gdb/common/enum-flags.h;hb=HEAD

> > > > 

> > > > I'll try this out with GCC; hopefully it works with C++98.

> > > 

> > > It should -- it was written/added while GDB still required C++98.

> > 

> > Thanks.

> > 

> > > Note I have a WIP series here:

> > > 

> > >  https://github.com/palves/gdb/commits/palves/cxx11-enum-flags

> > > 

> > > that fixes a few things, and adds a bunch of unit tests.  In

> > > the process, it uses C++11 constructs (hence the branch's name),

> > > but I think that can be reverted to still work with C++98.

> > > 

> > > I had seen some noises recently about GCC maybe considering

> > > requiring C++11.  Is that reasonable to expect in this cycle?

> > > (GDB requires GCC 4.8 or later, FWIW.)

> > 

> > I'm expecting that GCC 9 will still require C++98.

> 

> OK.

> 

> > 

> > > Getting that branch into master had fallen lower on my TODO,

> > > but if there's interest, I can try to finish it off soon, so we

> > > can share a better baseline.  (I posted it once, but found

> > > some issues which I fixed since but never managed to repost.)

> > 

> > Interestingly, it looks like gdb is using Google Test for

> > selftests;

> > gcc is using a hand-rolled API that is similar, but has numerous

> > differences (e.g. explicit calling of test functions, rather than

> > implicit test discovery).  So that's another area of drift between

> > the projects.

> 

> Nope, the unit tests framework is hand rolled too.  See

> gdb/selftest.h/c.

> It can be invoked with gdb's "maint selftest" command.

> You can see the list of tests with "maint info selftests", and

> you can pass a filter to "maint selftest" too.


Aha.  Thanks.  Looks like gdb and gcc each have different hand-rolled
selftest APIs:
* gdb's selftest.h/c has manual test registration; test failures are
handled via exceptions
* gcc's selftest.h/c has no test registration (test functions are
called explicitly); test failures are handled via "abort"

I'm not suggesting we try to unify these APIs at this time.

> > > > 

> > > > Presumably it would be good to share this header between GCC

> > > > and

> > > > GDB;

> > > > CCing Pedro; Pedro: hi!  Does this sound sane?

> > > > (for reference, the GCC patch we're discussing here is:

> > > >   https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html )

> > > 

> > > Sure!

> > 

> > Alternatively, if GCC needs to stay at C++98 and GDB moves on to

> > C++11,

> > then we can at least take advantage of this tested and FSF-assigned

> > C++98 code (better than writing it from scratch).

> 

> Agreed, but I'll try to see about making the fixes in the branch

> C++98 compatible.


Thanks.

> > 

> > I ran into one issue with the header, e.g. with:

> > 

> >   /* Dump flags type.  */

> >   DEF_ENUM_FLAGS_TYPE(enum dump_flag, dump_flags_t);

> > 

> > This doesn't work:

> >   TDF_SLIM | flags

> > but this does:

> >   flags | TDF_SLIM

> > where TDF_SLIM is one of the values of "enum dump_flag".

> 

> ISTR that that's fixed in the branch.


Interesting; I'll have a look.

> > BTW, I spotted this trivial issue in a comment in enum-flags.h

> > whilst

> > trying it out for gcc:

> > 

> > 

> > 

> > The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing

> > semicolon, but the example in the comment lacks one.

> > 

> > 	* enum-flags.h: Add trailing semicolon to example in comment.

> 

> Thanks.  I've merged it.


Thanks.

[...snip...]

Dave

Patch

diff --git a/include/enum-flags.h b/include/enum-flags.h
index 65732c1..82568a5 100644
--- a/include/enum-flags.h
+++ b/include/enum-flags.h
@@ -32,7 +32,7 @@ 
        flag_val3 = 1 << 3,
        flag_val4 = 1 << 4,
     };
-    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)
+    DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags);
 
     some_flags f = flag_val1 | flag_val2;
     f |= flag_val3;