[1/3] Fix mismatched struct vs class tags.
Commit Message
The 'collection_list' and 'number_or_range_parser' types were converted
from structs to classes, but some code still used 'struct'. Fix all
references to use 'class' which fixes -Wmismatched-tags warnings issued
by clang.
gdb/ChangeLog:
* breakpoint.h (class number_or_range_parser): Use 'class' instead of
'struct'.
* mi/mi-main.c (mi_cmd_trace_frame_collected): Use
'class collection_list' instead of 'struct collection_list'.
* tracepoint.c (class collection_list): Likewise.
(collection_list::collect_symbol): Likewise.
(encode_actions_1): Likewise.
(encode_actions_rsp): Likewise.
* tracepoint.h (encode_actions): Likewise.
---
gdb/ChangeLog | 12 ++++++++++++
gdb/breakpoint.h | 2 +-
gdb/mi/mi-main.c | 4 ++--
gdb/tracepoint.c | 14 +++++++-------
gdb/tracepoint.h | 4 ++--
5 files changed, 24 insertions(+), 12 deletions(-)
Comments
On 2016-11-23 15:06, John Baldwin wrote:
> The 'collection_list' and 'number_or_range_parser' types were converted
> from structs to classes, but some code still used 'struct'. Fix all
> references to use 'class' which fixes -Wmismatched-tags warnings issued
> by clang.
Whjen using the type in a parameter or variable declaration, should we
simply drop the keyword?
For example:
- struct collection_list *collect;
+ collection_list *collect;
That's the approach I took in my upcoming C++ patches, so I hope it's ok
:). I have also dropped the "enum" keyword when possible.
On Wednesday, November 23, 2016 03:58:11 PM Simon Marchi wrote:
> On 2016-11-23 15:06, John Baldwin wrote:
> > The 'collection_list' and 'number_or_range_parser' types were converted
> > from structs to classes, but some code still used 'struct'. Fix all
> > references to use 'class' which fixes -Wmismatched-tags warnings issued
> > by clang.
>
> Whjen using the type in a parameter or variable declaration, should we
> simply drop the keyword?
>
> For example:
>
> - struct collection_list *collect;
> + collection_list *collect;
>
> That's the approach I took in my upcoming C++ patches, so I hope it's ok
> :). I have also dropped the "enum" keyword when possible.
Hmm. I don't see anything about this in the GCC C++ language conventions,
so I will have to defer to others as far as what is the desired style here?
(And we should document whatever style is chosen)
On 11/23/2016 11:00 PM, John Baldwin wrote:
> On Wednesday, November 23, 2016 03:58:11 PM Simon Marchi wrote:
>> On 2016-11-23 15:06, John Baldwin wrote:
>>> The 'collection_list' and 'number_or_range_parser' types were converted
>>> from structs to classes, but some code still used 'struct'. Fix all
>>> references to use 'class' which fixes -Wmismatched-tags warnings issued
>>> by clang.
>>
>> Whjen using the type in a parameter or variable declaration, should we
>> simply drop the keyword?
>>
>> For example:
>>
>> - struct collection_list *collect;
>> + collection_list *collect;
>>
>> That's the approach I took in my upcoming C++ patches, so I hope it's ok
>> :). I have also dropped the "enum" keyword when possible.
>
> Hmm. I don't see anything about this in the GCC C++ language conventions,
> so I will have to defer to others as far as what is the desired style here?
> (And we should document whatever style is chosen)
I wouldn't say it's a matter of style to drop the "struct" or now.
It's just that we'll have legacy code using the explicit "struct"
style due to C heritage. Dropping it is fine. You can't drop it
in forward declarations, though.
I think I'd prefer a patch to add "-Wno-mismatched-tags" to the warning set.
This warning is useless for us. Forward declaring with "struct"
and defining with "class" is perfectly valid. That's useful as "struct"
vs "class" is just an implementation detail. IIRC, that clang
warning only exists because struct/class somehow makes a
difference with Microsoft's compilers (maybe it mangles
those differently, not sure), even though that's non conforming. But,
we don't support building with that.
Thanks,
Pedro Alves
On Thursday, November 24, 2016 05:02:07 PM Pedro Alves wrote:
> On 11/23/2016 11:00 PM, John Baldwin wrote:
> > On Wednesday, November 23, 2016 03:58:11 PM Simon Marchi wrote:
> >> On 2016-11-23 15:06, John Baldwin wrote:
> >>> The 'collection_list' and 'number_or_range_parser' types were converted
> >>> from structs to classes, but some code still used 'struct'. Fix all
> >>> references to use 'class' which fixes -Wmismatched-tags warnings issued
> >>> by clang.
> >>
> >> Whjen using the type in a parameter or variable declaration, should we
> >> simply drop the keyword?
> >>
> >> For example:
> >>
> >> - struct collection_list *collect;
> >> + collection_list *collect;
> >>
> >> That's the approach I took in my upcoming C++ patches, so I hope it's ok
> >> :). I have also dropped the "enum" keyword when possible.
> >
> > Hmm. I don't see anything about this in the GCC C++ language conventions,
> > so I will have to defer to others as far as what is the desired style here?
> > (And we should document whatever style is chosen)
>
> I wouldn't say it's a matter of style to drop the "struct" or now.
> It's just that we'll have legacy code using the explicit "struct"
> style due to C heritage. Dropping it is fine. You can't drop it
> in forward declarations, though.
>
> I think I'd prefer a patch to add "-Wno-mismatched-tags" to the warning set.
> This warning is useless for us. Forward declaring with "struct"
> and defining with "class" is perfectly valid. That's useful as "struct"
> vs "class" is just an implementation detail. IIRC, that clang
> warning only exists because struct/class somehow makes a
> difference with Microsoft's compilers (maybe it mangles
> those differently, not sure), even though that's non conforming. But,
> we don't support building with that.
Ok. At the moment we don't have a clang-specific warning set, but if we
add one we can add this to that.
On 11/24/2016 05:45 PM, John Baldwin wrote:
> Ok. At the moment we don't have a clang-specific warning set, but if we
> add one we can add this to that.
We likely don't need one. Our infrustruture checks whether a
warning works before enabling it. See gdb/warning.m4.
Thanks,
Pedro Alves
On Thursday, November 24, 2016 06:50:30 PM Pedro Alves wrote:
> On 11/24/2016 05:45 PM, John Baldwin wrote:
>
> > Ok. At the moment we don't have a clang-specific warning set, but if we
> > add one we can add this to that.
>
> We likely don't need one. Our infrustruture checks whether a
> warning works before enabling it. See gdb/warning.m4.
Hmmm. The only odd case I can think of is -Wunused-function. Right now
clang triggers warnings when VEC() is used, so ideally -Wunused-function
would only be present for GCC and not for clang. I can look at adding
other -Wno-foo warnings for clang things we wish to ignore however.
On 11/24/2016 07:15 PM, John Baldwin wrote:
> On Thursday, November 24, 2016 06:50:30 PM Pedro Alves wrote:
>> On 11/24/2016 05:45 PM, John Baldwin wrote:
>>
>>> Ok. At the moment we don't have a clang-specific warning set, but if we
>>> add one we can add this to that.
>>
>> We likely don't need one. Our infrustruture checks whether a
>> warning works before enabling it. See gdb/warning.m4.
>
> Hmmm. The only odd case I can think of is -Wunused-function. Right now
> clang triggers warnings when VEC() is used, so ideally -Wunused-function
Yeah, I still believe that's a clang bug, and clang developers
seem to agree:
https://llvm.org/bugs/show_bug.cgi?id=22712
> would only be present for GCC and not for clang. I can look at adding
> other -Wno-foo warnings for clang things we wish to ignore however.
Thanks,
Pedro Alves
On Wednesday, November 30, 2016 11:38:47 AM Pedro Alves wrote:
> On 11/24/2016 07:15 PM, John Baldwin wrote:
> > On Thursday, November 24, 2016 06:50:30 PM Pedro Alves wrote:
> >> On 11/24/2016 05:45 PM, John Baldwin wrote:
> >>
> >>> Ok. At the moment we don't have a clang-specific warning set, but if we
> >>> add one we can add this to that.
> >>
> >> We likely don't need one. Our infrustruture checks whether a
> >> warning works before enabling it. See gdb/warning.m4.
> >
> > Hmmm. The only odd case I can think of is -Wunused-function. Right now
> > clang triggers warnings when VEC() is used, so ideally -Wunused-function
>
> Yeah, I still believe that's a clang bug, and clang developers
> seem to agree:
>
> https://llvm.org/bugs/show_bug.cgi?id=22712
Oh certainly. My only point is that to get a -Werror clang build working
I'd need a way to exclude -Wunused-function from WARNFLAGS for clang. That's
the part I wasn't sure how to handle. I still need to see about adding
-Wno-foo for some other clang-only warnings to trim other bits of noise from
clang's build.
One other clangism is that clang warns about compiling a .c file in C++.
It wants an explicit '-x c++' to force the language mode. However, simply
adding this to CXX_FLAGS doesn't work as it is included in both compiling
and linking (and for the link it causes clang to try to parse all the object
files as C++ source leading to bizarre errors). I assume a massive .c -> .cc
(or .cxx, etc.) rename is not in the roadmap (it would presumably be very
disruptive to pending patchsets)?
On 11/30/2016 04:23 PM, John Baldwin wrote:
> Oh certainly. My only point is that to get a -Werror clang build working
> I'd need a way to exclude -Wunused-function from WARNFLAGS for clang. That's
> the part I wasn't sure how to handle. I still need to see about adding
> -Wno-foo for some other clang-only warnings to trim other bits of noise from
> clang's build.
Yeah, I think we'd need to add some "is this clang?" check somehow.
> One other clangism is that clang warns about compiling a .c file in C++.
> It wants an explicit '-x c++' to force the language mode. However, simply
> adding this to CXX_FLAGS doesn't work as it is included in both compiling
> and linking (and for the link it causes clang to try to parse all the object
> files as C++ source leading to bizarre errors).
Do you get the same when building GCC? If not, how is this handled
over there? Does clang have a way to suppress that warning?
Some -Wno-stop-complaining-about-c-file-in-cxx switch, perhaps?
> I assume a massive .c -> .cc
> (or .cxx, etc.) rename is not in the roadmap (it would presumably be very
> disruptive to pending patchsets)?
We briefly last discussed that here:
https://sourceware.org/ml/gdb-patches/2016-09/msg00309.html
The quick consensus was "no".
Thanks,
Pedro Alves
On 2016-11-30 11:23, John Baldwin wrote:
> One other clangism is that clang warns about compiling a .c file in
> C++.
> It wants an explicit '-x c++' to force the language mode. However,
> simply
> adding this to CXX_FLAGS doesn't work as it is included in both
> compiling
> and linking (and for the link it causes clang to try to parse all the
> object
> files as C++ source leading to bizarre errors).
I think you could add it in its own variable:
FORCE_LANG_FLAG = -x c++
and add that to INTERNAL_CFLAGS.
> I assume a massive .c -> .cc
> (or .cxx, etc.) rename is not in the roadmap (it would presumably be
> very
> disruptive to pending patchsets)?
I think it will have to be done at some point... it will be a bit weird
and counter intuitive for newcomers to see .c files containing C++.
That, and analysis tools that select the language based on the
extension. For example, I use Eclipse CDT for my development, and it
assumes C code for .c files by default. I can go change some obscure
setting to force it to consider it as C++, but it would be nicer for
everybody if we didn't have to do that. Actually, I just checked and
it's the same with vim and emacs. If we want to do it right, we would
have to rename .h into .hpp or .hh as well. And it would be as painful
to do it in 5 years as it would be to do it now, so I don't see why we
would wait...
About the merging of pending patches: if I try to apply a patch
including a change to a file that was renamed with "git am", it fails.
But if with "git rebase", git seems to handle it correctly. So there's
hope.
On 2016-11-30 11:38, Pedro Alves wrote:
> Do you get the same when building GCC? If not, how is this handled
> over there? Does clang have a way to suppress that warning?
> Some -Wno-stop-complaining-about-c-file-in-cxx switch, perhaps?
Double negative, such a flag would re-enable the complaining ;).
On 11/30/2016 04:50 PM, Simon Marchi wrote:
> On 2016-11-30 11:23, John Baldwin wrote:
>> One other clangism is that clang warns about compiling a .c file in C++.
>> It wants an explicit '-x c++' to force the language mode. However,
>> simply
>> adding this to CXX_FLAGS doesn't work as it is included in both compiling
>> and linking (and for the link it causes clang to try to parse all the
>> object
>> files as C++ source leading to bizarre errors).
>
> I think you could add it in its own variable:
>
> FORCE_LANG_FLAG = -x c++
>
> and add that to INTERNAL_CFLAGS.
>
Sounds like it'd work. At least for gcc and clang.
>> I assume a massive .c -> .cc
>> (or .cxx, etc.) rename is not in the roadmap (it would presumably be very
>> disruptive to pending patchsets)?
>
> I think it will have to be done at some point... it will be a bit weird
> and counter intuitive for newcomers to see .c files containing C++.
> That, and analysis tools that select the language based on the
> extension. For example, I use Eclipse CDT for my development, and it
> assumes C code for .c files by default. I can go change some obscure
> setting to force it to consider it as C++, but it would be nicer for
> everybody if we didn't have to do that. Actually, I just checked and
> it's the same with vim and emacs. If we want to do it right, we would
> have to rename .h into .hpp or .hh as well. And it would be as painful
> to do it in 5 years as it would be to do it now, so I don't see why we
> would wait...
".hh" and ".hpp" just look weird to me (for not being used to it,
no doubt). But how are these tools handling the massive number
of projects that use ".h" for C++ code?
( For emacs, we could put something in gdb/gdb-code-style.el / gdb/.dir-locals.el )
Thanks,
Pedro Alves
On 2016-11-30 12:08, Pedro Alves wrote:
>> I think it will have to be done at some point... it will be a bit
>> weird
>> and counter intuitive for newcomers to see .c files containing C++.
>> That, and analysis tools that select the language based on the
>> extension. For example, I use Eclipse CDT for my development, and it
>> assumes C code for .c files by default. I can go change some obscure
>> setting to force it to consider it as C++, but it would be nicer for
>> everybody if we didn't have to do that. Actually, I just checked and
>> it's the same with vim and emacs. If we want to do it right, we would
>> have to rename .h into .hpp or .hh as well. And it would be as
>> painful
>> to do it in 5 years as it would be to do it now, so I don't see why we
>> would wait...
>
> ".hh" and ".hpp" just look weird to me (for not being used to it,
> no doubt). But how are these tools handling the massive number
> of projects that use ".h" for C++ code?
In Eclipse CDT, if you create a C++ project, .h files will be treated as
C++, but .c files will be treated as C. vim seems to do always treat .h
files as C++. When I open "vim test.h", the file type is C++ right
away. So it would be fine in that regard to keep header files as .h.
> Date: Wed, 30 Nov 2016 11:50:59 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
>
> > I assume a massive .c -> .cc
> > (or .cxx, etc.) rename is not in the roadmap (it would presumably be
> > very
> > disruptive to pending patchsets)?
>
> I think it will have to be done at some point... it will be a bit weird
> and counter intuitive for newcomers to see .c files containing C++.
> That, and analysis tools that select the language based on the
> extension. For example, I use Eclipse CDT for my development, and it
> assumes C code for .c files by default. I can go change some obscure
> setting to force it to consider it as C++, but it would be nicer for
> everybody if we didn't have to do that. Actually, I just checked and
> it's the same with vim and emacs.
For Emacs, we could have a .dir-locals.el file in the tree which
instructed Emacs to enter C++ mode in the *.c and *.h files in our
tree.
@@ -1,3 +1,15 @@
+2016-11-23 John Baldwin <jhb@FreeBSD.org>
+
+ * breakpoint.h (class number_or_range_parser): Use 'class' instead of
+ 'struct'.
+ * mi/mi-main.c (mi_cmd_trace_frame_collected): Use
+ 'class collection_list' instead of 'struct collection_list'.
+ * tracepoint.c (class collection_list): Likewise.
+ (collection_list::collect_symbol): Likewise.
+ (encode_actions_1): Likewise.
+ (encode_actions_rsp): Likewise.
+ * tracepoint.h (encode_actions): Likewise.
+
2016-11-23 Pedro Alves <palves@redhat.com>
* Makefile.in (SFILES): Add common/run-time-clock.c.
@@ -32,7 +32,7 @@ struct value;
struct block;
struct gdbpy_breakpoint_object;
struct gdbscm_breakpoint_object;
-struct number_or_range_parser;
+class number_or_range_parser;
struct thread_info;
struct bpstats;
struct bp_location;
@@ -2763,8 +2763,8 @@ mi_cmd_trace_frame_collected (char *command, char **argv, int argc)
struct cleanup *old_chain;
struct bp_location *tloc;
int stepping_frame;
- struct collection_list *clist;
- struct collection_list tracepoint_list, stepping_list;
+ class collection_list *clist;
+ class collection_list tracepoint_list, stepping_list;
struct traceframe_info *tinfo;
int oind = 0;
enum print_values var_print_values = PRINT_ALL_VALUES;
@@ -180,7 +180,7 @@ static void trace_dump_command (char *, int);
/* support routines */
-struct collection_list;
+class collection_list;
static char *mem2hex (gdb_byte *, char *, int);
static struct command_line *
@@ -1079,7 +1079,7 @@ collection_list::collect_symbol (struct symbol *sym,
struct add_local_symbols_data
{
- struct collection_list *collect;
+ class collection_list *collect;
struct gdbarch *gdbarch;
CORE_ADDR pc;
long frame_regno;
@@ -1323,8 +1323,8 @@ encode_actions_1 (struct command_line *action,
struct bp_location *tloc,
int frame_reg,
LONGEST frame_offset,
- struct collection_list *collect,
- struct collection_list *stepping_list)
+ class collection_list *collect,
+ class collection_list *stepping_list)
{
const char *action_exp;
int i;
@@ -1553,8 +1553,8 @@ encode_actions_1 (struct command_line *action,
void
encode_actions (struct bp_location *tloc,
- struct collection_list *tracepoint_list,
- struct collection_list *stepping_list)
+ class collection_list *tracepoint_list,
+ class collection_list *stepping_list)
{
struct command_line *actions;
int frame_reg;
@@ -1578,7 +1578,7 @@ void
encode_actions_rsp (struct bp_location *tloc, char ***tdp_actions,
char ***stepping_actions)
{
- struct collection_list tracepoint_list, stepping_list;
+ class collection_list tracepoint_list, stepping_list;
*tdp_actions = NULL;
*stepping_actions = NULL;
@@ -323,8 +323,8 @@ void free_actions (struct breakpoint *);
extern const char *decode_agent_options (const char *exp, int *trace_string);
extern void encode_actions (struct bp_location *tloc,
- struct collection_list *tracepoint_list,
- struct collection_list *stepping_list);
+ class collection_list *tracepoint_list,
+ class collection_list *stepping_list);
extern void encode_actions_rsp (struct bp_location *tloc,
char ***tdp_actions, char ***stepping_actions);