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