On 1/11/19 4:21 PM, Marek Polacek wrote:
On Fri, Jan 11, 2019 at 01:55:09PM -0500, Jason Merrill wrote:
On 1/11/19 11:09 AM, Marek Polacek wrote:
On Mon, Jan 07, 2019 at 04:59:14PM -0500, Jason Merrill wrote:
On 1/7/19 4:29 PM, Marek Polacek wrote:
This patch fixes bogus -Wredundant-move warnings reported in 88692 and 87882.
To quickly recap, this warning is supposed to warn for cases like
struct T { };
T fn(T t)
{
return std::move (t);
}
where NRVO isn't applicable for T because it's a parameter, but it's
a local variable and we're returning, so C++11 says activate move
semantics, so the std::move is redundant. But, as these testcases show,
we're failing to realize that that is not the case when returning *this,
which is disguised as an ordinary PARM_DECL, and treat_lvalue_as_rvalue_p
was fooled by that.
Hmm, the function isn't returning 'this', it's returning '*this'. I guess
what's happening is that in order to pass *this to the reference parameter
of move, we end up converting it from pointer to reference by NOP_EXPR, and
the STRIP_NOPS in maybe_warn_pessimizing_move throws that away so that it
then thinks we're returning 'this'. I expect the same thing could happen
with any parameter of pointer-to-class type.
You're right, I didn't realize that we warned even for parameters of
pointer-to-class
types. So why don't we disable the warning for PARM_DECLs with pointer types?
std::move is certainly redundant for parms of pointer type (or other scalar
type), so we might still want to warn about 'return std::move(this)'. The
problem here is that we're discarding the indirection, so we aren't actually
considering the returned expression.
We won't warn for 'return std::move(this)' in any case becase
maybe_warn_pessimizing_move returns for non-class types. This is in line with
what clang does. I'm not sure if we should change that; I'd rather not.
Is the STRIP_NOPS really necessary? It seems we shouldn't remove a NOP_EXPR
from pointer to reference.
I think we need that in order to make can_do_nrvo_p/treat_lvalue_as_rvalue_p
work. Given the outermost NOP_EXPR will always be of reference type, how about
just returning if, after STRIP_NOPS, we don't see an ADDR_EXPR? That fixes the
testcases I've been meaning to fix and doesn't regress anything. I.e., this:
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -9412,8 +9412,9 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
{
tree arg = CALL_EXPR_ARG (fn, 0);
STRIP_NOPS (arg);
- if (TREE_CODE (arg) == ADDR_EXPR)
- arg = TREE_OPERAND (arg, 0);
+ if (TREE_CODE (arg) != ADDR_EXPR)
+ return;
+ arg = TREE_OPERAND (arg, 0);
arg = convert_from_reference (arg);
/* Warn if we could do copy elision were it not for the move. */
if (can_do_nrvo_p (arg, functype))
I can test/post the complete patch. Or am I again missing something obvious? :)
That looks good. OK if it passes.
Jason
