Bug 67906 - Missing warning about std::move without effect
Summary: Missing warning about std::move without effect
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 5.2.0
: P3 enhancement
Target Milestone: ---
Assignee: Marek Polacek
URL:
Keywords: diagnostic
Depends on:
Blocks: new-warning
  Show dependency treegraph
 
Reported: 2015-10-09 07:56 UTC by Nikolay Orliuk
Modified: 2022-08-17 19:46 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-08-24 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolay Orliuk 2015-10-09 07:56:25 UTC
It would be nice to have something like -Wno-effect which will issue warnings on usage of std::move for objects that cannot be moved.

See also http://stackoverflow.com/q/28595117/230744

I assume that it may require introducing differentiation between explicit-rvalue (through std::move()/cast) and implicit-rvalue.

This may save from errors like:

```
const Frame& x = stack.top();
Value y = std::move(x.value); // x.value - non-mutable
```

Though there might be a false-positives in template functions or when developer expects that in future function will have move overload for that argument.
Comment 1 Eric Gallager 2017-08-24 05:31:10 UTC
Could you please provide a complete self-contained example? The snippet you posted looks like it's missing some declarations:

$ /usr/local/bin/g++ -c 67906.cc
67906.cc:1:7: error: ‘Frame’ does not name a type
 const Frame& x = stack.top();
       ^~~~~
67906.cc:2:1: error: ‘Value’ does not name a type
 Value y = std::move(x.value); // x.value - non-mutable
 ^~~~~
$

Anyways, bug 81159 is related, but that's about a different misuse of std::move, so I'll keep the 2 separate.

Oh, and also "-Wno-effect" would probably be a bad name for the option, since "-Wno-" is reserved for negative versions of warnings. i.e., is "-Wno-effect" the negative of "-Weffect"? That seems wrong. Or is it already in the positive, in which case the negative is "-Wno-no-effect"? That would seem redundant.
Comment 2 Nikolay Orliuk 2017-08-24 19:38:26 UTC
Sure,

    struct Value {
        Value();
        Value(const Value&);
        Value(Value&&);
    };

    struct Frame {
        Value value; // previously mutable
    };

    Frame top;
    const Frame& x = top;
    Value y = std::move(x.value);


https://godbolt.org/g/v24FfQ

Thank you for looking into it. Yes, there should be better names than -Wno-effect. Maybe -Wignored-move and -Wmove-const are slightly better.

Such warning will help to identify places which become ineffecient after changing constness of something.
Comment 3 Nikolay Orliuk 2017-08-24 19:41:38 UTC
Sure,

    struct Value {
        Value();
        Value(const Value&);
        Value(Value&&);
    };

    struct Frame {
        Value value; // previously mutable
    };

    Frame top;
    const Frame& x = top;
    Value y = std::move(x.value);

Thank you for looking into it. Yes, there should be better names than -Wno-effect. Maybe -Wignored-move and -Wmove-const are slightly better.

Such warning will help to identify places which become ineffecient after changing constness of something.
Comment 4 Nikolay Orliuk 2017-08-24 19:47:00 UTC
Hello

Sure,

    struct Value {
        Value();
        Value(const Value&);
        Value(Value&&);
    };

    struct Frame {
        Value value; // previously mutable
    };

    Frame top;
    const Frame& x = top;
    Value y = std::move(x.value);


https://godbolt.org/g/v24FfQ

Thank you for looking into it. Yes, there should be better names than
-Wno-effect. Maybe -Wignored-move and -Wmove-const are slightly better.

Such warning will help to identify places which become ineffecient after
changing constness of something.

P.S. By some reason I were not able to leave comment in Bugzilla and got
message "User account creation filtered due to spam." though I were logged
in.

Thank you,
Mykola

On Thu, Aug 24, 2017 at 7:40 AM egallager at gcc dot gnu.org <
gcc-bugzilla@gcc.gnu.org> wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67906
>
> Eric Gallager <egallager at gcc dot gnu.org> changed:
>
>            What    |Removed                     |Added
>
> ----------------------------------------------------------------------------
>            Keywords|                            |diagnostic
>              Status|UNCONFIRMED                 |WAITING
>    Last reconfirmed|                            |2017-08-24
>                  CC|                            |egallager at gcc dot
> gnu.org
>      Ever confirmed|0                           |1
>
> --- Comment #1 from Eric Gallager <egallager at gcc dot gnu.org> ---
> Could you please provide a complete self-contained example? The snippet you
> posted looks like it's missing some declarations:
>
> $ /usr/local/bin/g++ -c 67906.cc
> 67906.cc:1:7: error: ‘Frame’ does not name a type
>  const Frame& x = stack.top();
>        ^~~~~
> 67906.cc:2:1: error: ‘Value’ does not name a type
>  Value y = std::move(x.value); // x.value - non-mutable
>  ^~~~~
> $
>
> Anyways, bug 81159 is related, but that's about a different misuse of
> std::move, so I'll keep the 2 separate.
>
> Oh, and also "-Wno-effect" would probably be a bad name for the option,
> since
> "-Wno-" is reserved for negative versions of warnings. i.e., is
> "-Wno-effect"
> the negative of "-Weffect"? That seems wrong. Or is it already in the
> positive,
> in which case the negative is "-Wno-no-effect"? That would seem redundant.
>
> --
> You are receiving this mail because:
> You reported the bug.
Comment 5 Eric Gallager 2017-08-24 19:48:58 UTC
Thanks. Testcase still needs:

#include <utility>

at the top, but once I've added that, confirmed that there's no warning about the usage of std::move.
Comment 6 Eric Gallager 2017-08-24 19:57:57 UTC
On 8/24/17, Mykola Orliuk <virkony@gmail.com> wrote:
> Hello
>
> Sure,
>
>     struct Value {
>         Value();
>         Value(const Value&);
>         Value(Value&&);
>     };
>
>     struct Frame {
>         Value value; // previously mutable
>     };
>
>     Frame top;
>     const Frame& x = top;
>     Value y = std::move(x.value);
>
>
> https://godbolt.org/g/v24FfQ
>
> Thank you for looking into it. Yes, there should be better names than
> -Wno-effect. Maybe -Wignored-move and -Wmove-const are slightly better.
>
> Such warning will help to identify places which become ineffecient after
> changing constness of something.
>
> P.S. By some reason I were not able to leave comment in Bugzilla and got
> message "User account creation filtered due to spam." though I were logged
> in.
>

No, your comment still went through, 3 times in fact. The "User
account creation filtered due to spam" message is pinned to the top
for display to everyone; it doesn't actually mean that anything is
wrong with your account.

> Thank you,
> Mykola
>
> On Thu, Aug 24, 2017 at 7:40 AM egallager at gcc dot gnu.org <
> gcc-bugzilla@gcc.gnu.org> wrote:
>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67906
>>
>> Eric Gallager <egallager at gcc dot gnu.org> changed:
>>
>>            What    |Removed                     |Added
>>
>> ----------------------------------------------------------------------------
>>            Keywords|                            |diagnostic
>>              Status|UNCONFIRMED                 |WAITING
>>    Last reconfirmed|                            |2017-08-24
>>                  CC|                            |egallager at gcc dot
>> gnu.org
>>      Ever confirmed|0                           |1
>>
>> --- Comment #1 from Eric Gallager <egallager at gcc dot gnu.org> ---
>> Could you please provide a complete self-contained example? The snippet
>> you
>> posted looks like it's missing some declarations:
>>
>> $ /usr/local/bin/g++ -c 67906.cc
>> 67906.cc:1:7: error: ‘Frame’ does not name a type
>>  const Frame& x = stack.top();
>>        ^~~~~
>> 67906.cc:2:1: error: ‘Value’ does not name a type
>>  Value y = std::move(x.value); // x.value - non-mutable
>>  ^~~~~
>> $
>>
>> Anyways, bug 81159 is related, but that's about a different misuse of
>> std::move, so I'll keep the 2 separate.
>>
>> Oh, and also "-Wno-effect" would probably be a bad name for the option,
>> since
>> "-Wno-" is reserved for negative versions of warnings. i.e., is
>> "-Wno-effect"
>> the negative of "-Weffect"? That seems wrong. Or is it already in the
>> positive,
>> in which case the negative is "-Wno-no-effect"? That would seem
>> redundant.
>>
>> --
>> You are receiving this mail because:
>> You reported the bug.
>
Comment 7 Jonathan Wakely 2017-08-29 12:47:23 UTC
I don't think this is a useful enhancement request as currently written.

What are the precise semantics of the new warning that you want? Which cases should warn, and which should not?

"std::move with no effect" is not clear enough, because the intended effect of std::move is simply to cast to an rvalue, and it always does that. A type that only has a copy constructor and no move constructor could be used in generic code, where that case "has no effect" but that doesn't mean it should warn e.g.

struct A {
  A { }
  A(const X&) { }
};

template<typename T> struct identity { using type = T; };

template<typename T>
bool f(typename identity<T>::type&& t)
{
  T other = std::move(t);
  /* ... */
  return true;
}

A a;
bool b = f<A>(a);

This presumably shouldn't warn.

Should there be a warning for the following code?

struct X {
  X() { }
  X(const X&) { }
  X(const X&&) { }
};

const X x;
const X y = std::move(x);

Although constructors taking a const rvalue reference are rare and not useful in practice, this code has entirely well-defined meaning. Should there be a warning?
Comment 8 Jonathan Wakely 2018-08-16 15:05:10 UTC
Kinda related: PR 86981
Comment 9 Nikolay Orliuk 2018-08-16 22:32:49 UTC
Yes. Return value optimization blocked by abusing std::move also close. Somehow I had feeling that I saw something similar from gcc in pre-C++11 times.

Regarding sample:

struct X {
  X() { }
  X(const X&) { }
  X(const X&&) { }
};

const X x;
const X y = std::move(x);

It shouldn't warn because std::move is effective here and makes choice between default (const X&) and (const X&&) overloads. I.e. we are not ignoring the fact that it is rvalue.

As I stated initially I agree that most likely templates will cause false triggering. Like for other warnings (ex. -Wduplicated-branches ).

The main point here was to identify places that potentially migrated incompletely. Kinda temporary flag.
Trigger rule might be: cast to rvalue for overloads one of which have slightly different qualifiers combinations (e.g. const vs non-const).
That should a bit reduce chances of false trigerring compared to "whenever we ignore rvalue".
If someone will want more noise they can request adding -Wsuggest-move-overload  to supported flags :)
Comment 10 Eric Gallager 2019-04-17 21:33:42 UTC
(In reply to Jonathan Wakely from comment #8)
> Kinda related: PR 86981

cc-ing Marek since he fixed that
Comment 11 Marek Polacek 2022-08-04 14:59:23 UTC
I'm going to add a testcase that ensures that we don't warn for

struct X {
  X() { }
  X(const X&) { }
  X(const X&&) { }
};

const X x;
const X y = std::move(x);

but otherwise I don't see any useful extension to our std::move warnings.
Comment 12 CVS Commits 2022-08-17 19:46:08 UTC
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:177e93e95272e9b373203dee5b28d2b284ffa05e

commit r13-2099-g177e93e95272e9b373203dee5b28d2b284ffa05e
Author: Marek Polacek <polacek@redhat.com>
Date:   Wed Aug 17 15:43:45 2022 -0400

    c++: Add new std::move test [PR67906]
    
    As discussed in 67906, let's make sure we don't warn about a std::move
    when initializing when there's a T(const T&&) ctor.
    
            PR c++/67906
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp0x/Wredundant-move11.C: New test.
Comment 13 Marek Polacek 2022-08-17 19:46:50 UTC
Test added.