On Fri, Jan 11, 2019 at 04:23:23PM -0500, Jason Merrill wrote:
> 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.
Which it did, so I'm installing the following. Thanks!
2019-01-11 Marek Polacek <pola...@redhat.com>
PR c++/88692, c++/87882 - -Wredundant-move false positive with *this.
* typeck.c (maybe_warn_pessimizing_move): Return if ARG isn't
ADDR_EXPR.
* g++.dg/cpp0x/Wredundant-move5.C: New test.
* g++.dg/cpp0x/Wredundant-move6.C: New test.
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index e399cd3fe45..43d2899a3c4 100644
--- gcc/cp/typeck.c
+++ 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))
diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move5.C
gcc/testsuite/g++.dg/cpp0x/Wredundant-move5.C
new file mode 100644
index 00000000000..0e2ec46d11e
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move5.C
@@ -0,0 +1,53 @@
+// PR c++/88692
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wredundant-move" }
+
+// Define std::move.
+namespace std {
+ template<typename _Tp>
+ struct remove_reference
+ { typedef _Tp type; };
+
+ template<typename _Tp>
+ struct remove_reference<_Tp&>
+ { typedef _Tp type; };
+
+ template<typename _Tp>
+ struct remove_reference<_Tp&&>
+ { typedef _Tp type; };
+
+ template<typename _Tp>
+ constexpr typename std::remove_reference<_Tp>::type&&
+ move(_Tp&& __t) noexcept
+ { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
+}
+
+struct X {
+ X f() && {
+ return std::move(*this); // { dg-bogus "redundant move in return
statement" }
+ }
+
+ X f2() & {
+ return std::move(*this); // { dg-bogus "redundant move in return
statement" }
+ }
+
+ X f3() {
+ return std::move(*this); // { dg-bogus "redundant move in return
statement" }
+ }
+};
+
+struct S { int i; int j; };
+
+struct Y {
+ S f1 (S s) {
+ return std::move (s); // { dg-warning "redundant move in return statement"
}
+ }
+
+ S f2 (S* s) {
+ return std::move (*s); // { dg-bogus "redundant move in return statement" }
+ }
+
+ S f3 (S** s) {
+ return std::move (**s); // { dg-bogus "redundant move in return statement"
}
+ }
+};
diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move6.C
gcc/testsuite/g++.dg/cpp0x/Wredundant-move6.C
new file mode 100644
index 00000000000..5808a78638e
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move6.C
@@ -0,0 +1,43 @@
+// PR c++/87882
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wredundant-move" }
+
+// Define std::move.
+namespace std {
+ template<typename _Tp>
+ struct remove_reference
+ { typedef _Tp type; };
+
+ template<typename _Tp>
+ struct remove_reference<_Tp&>
+ { typedef _Tp type; };
+
+ template<typename _Tp>
+ struct remove_reference<_Tp&&>
+ { typedef _Tp type; };
+
+ template<typename _Tp>
+ constexpr typename std::remove_reference<_Tp>::type&&
+ move(_Tp&& __t) noexcept
+ { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
+}
+
+struct Foo {
+ Foo Bar() {
+ return std::move(*this); // { dg-bogus "redundant move in return
statement" }
+ }
+ Foo Baz() {
+ return *this;
+ }
+ int i;
+};
+
+void Move(Foo & f)
+{
+ f = Foo{}.Bar();
+}
+
+void NoMove(Foo & f)
+{
+ f = Foo{}.Baz();
+}
