[PATCH 0/3] Fix various C++ related clang warnings

[PATCH 0/3] Fix various C++ related clang warnings

[John Baldwin]

These patches fix various new C++ warnings reported by clang 3.8.0.
The last one regarding std::move is a bit surprising I think, but
there's a not-bad answer to a similar issue here:

http://stackoverflow.com/questions/19267408/why-does-stdmove-prevent-rvo

John Baldwin (3):
  Fix mismatched struct vs class tags.
  Add noexcept to custom non-throwing new operators.
  Do not use std::move when assigning an anonymous object to a
    unique_ptr.

 gdb/ChangeLog       | 27 +++++++++++++++++++++++++++
 gdb/ada-lang.c      |  6 +++---
 gdb/ax-gdb.c        |  8 ++++----
 gdb/breakpoint.c    |  8 ++++----
 gdb/breakpoint.h    |  2 +-
 gdb/common/new-op.c |  4 ++--
 gdb/dtrace-probe.c  |  3 +--
 gdb/mi/mi-main.c    |  4 ++--
 gdb/parse.c         |  2 +-
 gdb/tracepoint.c    | 14 +++++++-------
 gdb/tracepoint.h    |  4 ++--
 11 files changed, 54 insertions(+), 28 deletions(-)

-- 
2.9.2

[PATCH 3/3] Do not use std::move when assigning an anonymous object to a unique_ptr.

[John Baldwin]

Using std::move forces an extra copy of the object.  These changes fix
-Wpessimizing-move warnings from clang.

gdb/ChangeLog:

	* gdb/ada-lang.c (create_excep_cond_exprs): Do not use 'std::move'.
	* gdb/ax-gdb.c (agent_eval_command_one): Likewise.
	(agent_eval_command_one): Likewise.
	* gdb/breakpoint.c (parse_cond_to_aexpr): Likewise.
	(parse_cmd_to_aexpr): Likewise.
	* gdb/dtrace-probe.c (dtrace_process_dof_probe): Likewise.
	* gdb/parse.c (parse_expression_for_completion): Likewise.
---
 gdb/ChangeLog      | 10 ++++++++++
 gdb/ada-lang.c     |  6 +++---
 gdb/ax-gdb.c       |  8 ++++----
 gdb/breakpoint.c   |  8 ++++----
 gdb/dtrace-probe.c |  3 +--
 gdb/parse.c        |  2 +-
 6 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 051d4ce..d331edf 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2016-11-23  John Baldwin  <jhb@FreeBSD.org>
 
+	* gdb/ada-lang.c (create_excep_cond_exprs): Do not use 'std::move'.
+	* gdb/ax-gdb.c (agent_eval_command_one): Likewise.
+	(agent_eval_command_one): Likewise.
+	* gdb/breakpoint.c (parse_cond_to_aexpr): Likewise.
+	(parse_cmd_to_aexpr): Likewise.
+	* gdb/dtrace-probe.c (dtrace_process_dof_probe): Likewise.
+	* gdb/parse.c (parse_expression_for_completion): Likewise.
+
+2016-11-23  John Baldwin  <jhb@FreeBSD.org>
+
 	* common/new-op.c (operator new): Mark 'noexcept'.
 	(operator new[]): Likewise.
 
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 0647a9b..78c7d6f 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12343,9 +12343,9 @@ create_excep_cond_exprs (struct ada_catchpoint *c)
 	  s = cond_string;
 	  TRY
 	    {
-	      exp = std::move (parse_exp_1 (&s, bl->address,
-					    block_for_pc (bl->address),
-					    0));
+	      exp = parse_exp_1 (&s, bl->address,
+				 block_for_pc (bl->address),
+				 0);
 	    }
 	  CATCH (e, RETURN_MASK_ERROR)
 	    {
diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index cd97585..49108de 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -2555,8 +2555,8 @@ agent_eval_command_one (const char *exp, int eval, CORE_ADDR pc)
   arg = exp;
   if (!eval && strcmp (arg, "$_ret") == 0)
     {
-      agent = std::move (gen_trace_for_return_address (pc, get_current_arch (),
-						       trace_string));
+      agent = gen_trace_for_return_address (pc, get_current_arch (),
+					    trace_string);
     }
   else
     {
@@ -2565,10 +2565,10 @@ agent_eval_command_one (const char *exp, int eval, CORE_ADDR pc)
       if (eval)
 	{
 	  gdb_assert (trace_string == 0);
-	  agent = std::move (gen_eval_for_expr (pc, expr.get ()));
+	  agent = gen_eval_for_expr (pc, expr.get ());
 	}
       else
-	agent = std::move (gen_trace_for_expr (pc, expr.get (), trace_string));
+	agent = gen_trace_for_expr (pc, expr.get (), trace_string);
     }
 
   ax_reqs (agent.get ());
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 67b610c..e2fcc08 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2268,7 +2268,7 @@ parse_cond_to_aexpr (CORE_ADDR scope, struct expression *cond)
      that may show up.  */
   TRY
     {
-      aexpr = std::move (gen_eval_for_expr (scope, cond));
+      aexpr = gen_eval_for_expr (scope, cond);
     }
 
   CATCH (ex, RETURN_MASK_ERROR)
@@ -2452,9 +2452,9 @@ parse_cmd_to_aexpr (CORE_ADDR scope, char *cmd)
      that may show up.  */
   TRY
     {
-      aexpr = std::move (gen_printf (scope, gdbarch, 0, 0,
-				     format_start, format_end - format_start,
-				     fpieces, nargs, argvec));
+      aexpr = gen_printf (scope, gdbarch, 0, 0,
+			  format_start, format_end - format_start,
+			  fpieces, nargs, argvec);
     }
   CATCH (ex, RETURN_MASK_ERROR)
     {
diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 38654a4..29c2d28 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -430,8 +430,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
 
 	  TRY
 	    {
-	      expr = std::move (parse_expression_with_language (arg.type_str,
-								language_c));
+	      expr = parse_expression_with_language (arg.type_str, language_c);
 	    }
 	  CATCH (ex, RETURN_MASK_ERROR)
 	    {
diff --git a/gdb/parse.c b/gdb/parse.c
index afafc35..323de77 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -1309,7 +1309,7 @@ parse_expression_for_completion (const char *string, char **name,
   TRY
     {
       parse_completion = 1;
-      exp = std::move (parse_exp_in_context (&string, 0, 0, 0, 0, &subexp));
+      exp = parse_exp_in_context (&string, 0, 0, 0, 0, &subexp);
     }
   CATCH (except, RETURN_MASK_ERROR)
     {
-- 
2.9.2

Re: [PATCH 3/3] Do not use std::move when assigning an anonymous object to a unique_ptr.

[Simon Marchi]

On 2016-11-23 15:06, John Baldwin wrote:
> Using std::move forces an extra copy of the object.  These changes fix
> -Wpessimizing-move warnings from clang.

For those who, like me, do not quite understand what is happening here, 
I suggest the following read:

https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en

Re: [PATCH 3/3] Do not use std::move when assigning an anonymous object to a unique_ptr.

[John Baldwin]

On Wednesday, November 23, 2016 04:19:29 PM Simon Marchi wrote:
> On 2016-11-23 15:06, John Baldwin wrote:
> > Using std::move forces an extra copy of the object.  These changes fix
> > -Wpessimizing-move warnings from clang.
> 
> For those who, like me, do not quite understand what is happening here, 
> I suggest the following read:
> 
> https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en

My head also hurts.  I think what clang is warning about is that the
std::move() in these lines breaks RVO for the function being called,
not the function that the modified line belongs to.  That is:

	foo = bar ();

Is able to do RVO if bar() does the right things for RVO to work.
However:

	foo = std::move (bar ());

forces an extra copy of the object since the return value of bar
can't use the storge of 'foo' directly, it has to be copied into
an anonymous object (I think) for std::move to consume.

The commit log for the warning is here:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150427/128053.html

I think these instances fall under the "using a move to create a new object
from a temporary object" case.

-- 
John Baldwin

Re: [PATCH 3/3] Do not use std::move when assigning an anonymous object to a unique_ptr.

[Simon Marchi]

On 2016-11-23 18:31, John Baldwin wrote:
> On Wednesday, November 23, 2016 04:19:29 PM Simon Marchi wrote:
>> On 2016-11-23 15:06, John Baldwin wrote:
>> > Using std::move forces an extra copy of the object.  These changes fix
>> > -Wpessimizing-move warnings from clang.
>> 
>> For those who, like me, do not quite understand what is happening 
>> here,
>> I suggest the following read:
>> 
>> https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en
> 
> My head also hurts.  I think what clang is warning about is that the
> std::move() in these lines breaks RVO for the function being called,
> not the function that the modified line belongs to.  That is:
> 
> 	foo = bar ();
> 
> Is able to do RVO if bar() does the right things for RVO to work.
> However:
> 
> 	foo = std::move (bar ());
> 
> forces an extra copy of the object since the return value of bar
> can't use the storge of 'foo' directly, it has to be copied into
> an anonymous object (I think) for std::move to consume.
> 
> The commit log for the warning is here:
> 
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150427/128053.html
> 
> I think these instances fall under the "using a move to create a new 
> object
> from a temporary object" case.

That's what I understand.  Without the move, the object is constructed 
directly in the caller's stack, so no move/copy is required at all.  It 
seems like the warning works as intended and is useful.

Re: [PATCH 3/3] Do not use std::move when assigning an anonymous object to a unique_ptr.

[Pedro Alves]

On 11/24/2016 12:08 AM, Simon Marchi wrote:
> On 2016-11-23 18:31, John Baldwin wrote:
>> On Wednesday, November 23, 2016 04:19:29 PM Simon Marchi wrote:
>>> On 2016-11-23 15:06, John Baldwin wrote:
>>> > Using std::move forces an extra copy of the object.  These changes fix
>>> > -Wpessimizing-move warnings from clang.
>>>
>>> For those who, like me, do not quite understand what is happening here,
>>> I suggest the following read:
>>>
>>> https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en

I'd recommend as well:

  https://en.wikipedia.org/wiki/Return_value_optimization
  http://en.cppreference.com/w/cpp/language/copy_elision

Note that C++17 has much stronger copy-elision guarantees.

>>>
>>
>> My head also hurts.  I think what clang is warning about is that the
>> std::move() in these lines breaks RVO for the function being called,
>> not the function that the modified line belongs to.  That is:
>>
>>     foo = bar ();
>>
>> Is able to do RVO if bar() does the right things for RVO to work.
>> However:
>>
>>     foo = std::move (bar ());
>>
>> forces an extra copy of the object since the return value of bar
>> can't use the storge of 'foo' directly, it has to be copied into
>> an anonymous object (I think) for std::move to consume.
>>
>> The commit log for the warning is here:
>>
>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150427/128053.html
>>
>>
>> I think these instances fall under the "using a move to create a new
>> object
>> from a temporary object" case.
> 
> That's what I understand.  Without the move, the object is constructed
> directly in the caller's stack, so no move/copy is required at all.  It
> seems like the warning works as intended and is useful.

I've been harping on exploring RVO in several patches/reviews,
so I'm surprised I added those std::move calls in the first place.  :-P

Maybe something to do with an earlier version of gdb::unique_ptr.

Anyway, removing them is really right thing to do.

Patch is OK, but please drop the leading "gdb/" in filenames
in the ChangeLog.

Thanks,
Pedro Alves

[PATCH 1/3] Fix mismatched struct vs class tags.

[John Baldwin]

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(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a2a11e2..9e8fb4f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -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.
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 99133a2..111e37a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -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;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 4d276c8..edc1857 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -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;
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 7435380..0827f92 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -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;
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 36eeee6..dfb85c8 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -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);
-- 
2.9.2

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Simon Marchi]

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.

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[John Baldwin]

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)

-- 
John Baldwin

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Pedro Alves]

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

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[John Baldwin]

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.

-- 
John Baldwin

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Pedro Alves]

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

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[John Baldwin]

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.

-- 
John Baldwin

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Pedro Alves]

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

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[John Baldwin]

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)?

-- 
John Baldwin

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Pedro Alves]

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

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Simon Marchi]

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 ;).

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Simon Marchi]

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.

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Pedro Alves]

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

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Simon Marchi]

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.

Re: [PATCH 1/3] Fix mismatched struct vs class tags.

[Eli Zaretskii]

> 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.

[PATCH 2/3] Add noexcept to custom non-throwing new operators.

[John Baldwin]

Both libc++ and libstdc++ declare non-throwing new operators as
noexcept and overloads must also be noexcept.  This fixes a
-Wmissing-exception-spec warning with clang.

gdb/ChangeLog:

	* common/new-op.c (operator new): Mark 'noexcept'.
	(operator new[]): Likewise.
---
 gdb/ChangeLog       | 5 +++++
 gdb/common/new-op.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9e8fb4f..051d4ce 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2016-11-23  John Baldwin  <jhb@FreeBSD.org>
 
+	* common/new-op.c (operator new): Mark 'noexcept'.
+	(operator new[]): Likewise.
+
+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
diff --git a/gdb/common/new-op.c b/gdb/common/new-op.c
index 1eb4f94..c67239c 100644
--- a/gdb/common/new-op.c
+++ b/gdb/common/new-op.c
@@ -76,7 +76,7 @@ operator new (std::size_t sz)
 }
 
 void *
-operator new (std::size_t sz, const std::nothrow_t&)
+operator new (std::size_t sz, const std::nothrow_t&) noexcept
 {
   /* malloc (0) is unpredictable; avoid it.  */
   if (sz == 0)
@@ -91,7 +91,7 @@ operator new[] (std::size_t sz)
 }
 
 void*
-operator new[] (std::size_t sz, const std::nothrow_t&)
+operator new[] (std::size_t sz, const std::nothrow_t&) noexcept
 {
   return ::operator new (sz, std::nothrow);
 }
-- 
2.9.2

Re: [PATCH 2/3] Add noexcept to custom non-throwing new operators.

[Pedro Alves]

On 11/23/2016 08:06 PM, John Baldwin wrote:
> Both libc++ and libstdc++ declare non-throwing new operators as
> noexcept and overloads must also be noexcept.  This fixes a
> -Wmissing-exception-spec warning with clang.
> 
> gdb/ChangeLog:
> 
> 	* common/new-op.c (operator new): Mark 'noexcept'.
> 	(operator new[]): Likewise.

OK.

Thanks,
Pedro Alves

Re: [PATCH 0/3] Fix various C++ related clang warnings

[Simon Marchi]

On 2016-11-23 15:06, John Baldwin wrote:
> These patches fix various new C++ warnings reported by clang 3.8.0.
> The last one regarding std::move is a bit surprising I think, but
> there's a not-bad answer to a similar issue here:
> 
> http://stackoverflow.com/questions/19267408/why-does-stdmove-prevent-rvo
> 
> John Baldwin (3):
>   Fix mismatched struct vs class tags.
>   Add noexcept to custom non-throwing new operators.
>   Do not use std::move when assigning an anonymous object to a
>     unique_ptr.

Hi John,

When I tried to build gdb and gdbserver on Linux with clang, I got many 
more warnings/errors (often relevant).  I assume you have many more 
fixups to do to actually get it building on FreeBSD with clang?

Simon

Re: [PATCH 0/3] Fix various C++ related clang warnings

[John Baldwin]

On Wednesday, November 23, 2016 05:18:28 PM Simon Marchi wrote:
> On 2016-11-23 15:06, John Baldwin wrote:
> > These patches fix various new C++ warnings reported by clang 3.8.0.
> > The last one regarding std::move is a bit surprising I think, but
> > there's a not-bad answer to a similar issue here:
> > 
> > http://stackoverflow.com/questions/19267408/why-does-stdmove-prevent-rvo
> > 
> > John Baldwin (3):
> >   Fix mismatched struct vs class tags.
> >   Add noexcept to custom non-throwing new operators.
> >   Do not use std::move when assigning an anonymous object to a
> >     unique_ptr.
> 
> Hi John,
> 
> When I tried to build gdb and gdbserver on Linux with clang, I got many 
> more warnings/errors (often relevant).  I assume you have many more 
> fixups to do to actually get it building on FreeBSD with clang?

I build without -Werror when using clang.  There are several more warnings
beyond these.  One set in particular is a set of -Wunused-function that
get generated by VEC().  My understanding is that VEC() will be replaced by
templates at some point which will trim many of the current clang warnings.
There are several tautological compare warnings due to doing
'if (foo >= 0 && foo <= X)' when 'foo' is unsigned (the comparison against
0 is always true in that case).  However, I sometimes think that listing
the explicit bounds can be useful to the reader in those cases so have
been hesitant to change those.  One option we could take is to provide
different WARN_CFLAGS for GCC vs clang, but I haven't sat down to figure out
how to do that.  I do try to fix the warnings that I think are relevant
however (such as in this set).

-- 
John Baldwin