diff mbox

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

Message ID 20161123200652.89209-4-jhb@FreeBSD.org
State New, archived
Headers

Commit Message

John Baldwin Nov. 23, 2016, 8:06 p.m. UTC 
  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(-)

Comments

Simon Marchi Nov. 23, 2016, 9:19 p.m. UTC | #1 
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
John Baldwin Nov. 23, 2016, 11:31 p.m. UTC | #2 
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.
Simon Marchi Nov. 24, 2016, 12:08 a.m. UTC | #3 
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.
Pedro Alves Nov. 24, 2016, 4:52 p.m. UTC | #4 
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
diff mbox

Patch

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