* C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
@ 2019-01-07 21:29 Marek Polacek
2019-01-07 21:59 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2019-01-07 21:29 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
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.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2019-01-07 Marek Polacek <polacek@redhat.com>
PR c++/88692, c++/87882 - -Wredundant-move false positive with *this.
* typeck.c (treat_lvalue_as_rvalue_p): Return false for 'this'.
* 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..c6908d23a11 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9371,6 +9371,9 @@ bool
treat_lvalue_as_rvalue_p (tree retval, bool parm_ok)
{
STRIP_ANY_LOCATION_WRAPPER (retval);
+ /* *this remains an lvalue expression. */
+ if (is_this_parameter (retval))
+ return false;
return ((cxx_dialect != cxx98)
&& ((VAR_P (retval) && !DECL_HAS_VALUE_EXPR_P (retval))
|| (parm_ok && TREE_CODE (retval) == PARM_DECL))
diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move5.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move5.C
new file mode 100644
index 00000000000..b6a3b2296a8
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move5.C
@@ -0,0 +1,37 @@
+// 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" }
+ }
+};
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();
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
2019-01-07 21:29 C++ PATCH for c++/88692 - -Wredundant-move false positive with *this Marek Polacek
@ 2019-01-07 21:59 ` Jason Merrill
2019-01-11 16:09 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2019-01-07 21:59 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
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.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
2019-01-07 21:59 ` Jason Merrill
@ 2019-01-11 16:09 ` Marek Polacek
2019-01-11 18:55 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2019-01-11 16:09 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
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?
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2019-01-11 Marek Polacek <polacek@redhat.com>
PR c++/88692 - -Wredundant-move false positive with *this.
* typeck.c (maybe_warn_pessimizing_move): Don't issue Wredundant-move
warnings for variables of pointer types.
* 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..2b26e49f676 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9426,7 +9426,8 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
}
/* Warn if the move is redundant. It is redundant when we would
do maybe-rvalue overload resolution even without std::move. */
- else if (treat_lvalue_as_rvalue_p (arg, /*parm_ok*/true))
+ else if (!POINTER_TYPE_P (TREE_TYPE (arg))
+ && treat_lvalue_as_rvalue_p (arg, /*parm_ok*/true))
{
auto_diagnostic_group d;
if (warning_at (loc, OPT_Wredundant_move,
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();
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
2019-01-11 16:09 ` Marek Polacek
@ 2019-01-11 18:55 ` Jason Merrill
2019-01-11 21:22 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2019-01-11 18:55 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
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.
Is the STRIP_NOPS really necessary? It seems we shouldn't remove a
NOP_EXPR from pointer to reference.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
2019-01-11 18:55 ` Jason Merrill
@ 2019-01-11 21:22 ` Marek Polacek
2019-01-11 21:23 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2019-01-11 21:22 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
2019-01-11 21:22 ` Marek Polacek
@ 2019-01-11 21:23 ` Jason Merrill
2019-01-11 23:18 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2019-01-11 21:23 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH for c++/88692 - -Wredundant-move false positive with *this
2019-01-11 21:23 ` Jason Merrill
@ 2019-01-11 23:18 ` Marek Polacek
0 siblings, 0 replies; 7+ messages in thread
From: Marek Polacek @ 2019-01-11 23:18 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
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 <polacek@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();
+}
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-11 23:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-07 21:29 C++ PATCH for c++/88692 - -Wredundant-move false positive with *this Marek Polacek
2019-01-07 21:59 ` Jason Merrill
2019-01-11 16:09 ` Marek Polacek
2019-01-11 18:55 ` Jason Merrill
2019-01-11 21:22 ` Marek Polacek
2019-01-11 21:23 ` Jason Merrill
2019-01-11 23:18 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).