[3/3] Do not use std::move when assigning an anonymous object to a unique_ptr.
Commit Message
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
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
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.
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.
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
@@ -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.
@@ -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)
{
@@ -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 ());
@@ -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)
{
@@ -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)
{
@@ -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)
{