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

login
register
mail settings
Submitter Pedro Alves
Date June 5, 2018, 5:31 p.m.
Message ID <8f9ccb7e-27cb-63a1-05d4-044b00a078d3@redhat.com>
Download mbox | patch
Permalink /patch/27636/
State New
Headers show

Comments

Pedro Alves - June 5, 2018, 5:31 p.m.
[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(-)
David Malcolm - June 6, 2018, 12:54 p.m.
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/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;