[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #2 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
I've posted a patch for this:
https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599342.html

Sorry it's taken so long.

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[herring at lanl dot gov]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #3 from S. Davis Herring <herring at lanl dot gov> ---
Does this need to be language-version-dependent, given
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html (in
C++20) and
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2266r3.html (in
C++23)?

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #4 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to S. Davis Herring from comment #3)
> Does this need to be language-version-dependent, given
> https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html (in
> C++20) and
> https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2266r3.html (in
> C++23)?

So far I couldn't find a testcase where we either
1) say that a std::move is pessimizing while it's only redundant, or
2) don't say that a std::move is redundant when it is, under the new rules.

I don't think we fully implement p2266r3 yet, maybe once we do, the warning
will have to be adjusted.

For instance, this

struct S1 { S1(); S1(S1 &&); };
struct S2 : S1 {};

S1
f (S2 s)
{
  return std::move(s);
}

with -std=c++20 will warn about a redundant move in the return statement.  In
C++17 it will not compile because the deleted S1(const S1&) is used.

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[herring at lanl dot gov]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #5 from S. Davis Herring <herring at lanl dot gov> ---
Perhaps I'm misunderstanding something, but your example (as well as compiling
the original example with -std=c++20, which produces the same warning but now
calls Dest(Dest&&) in the noMove<Source> case) means that the warning is
correct in C++20 mode (but not in C++17).  If that's the case, why suppress the
warning in formerly dependent contexts regardless of language version?

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
The warning warns about dubious uses of std::move, so in the noMove case we
won't warn at all since there's no std::move.
In the withMove case, in C++20, we issue:
warning: moving a local object in a return statement prevents copy elision
for
template Dest withMove<Dest>();
and:
warning: redundant move in return statement
for
template Dest withMove<Source>();

In C++17, we issue one (wrong) warning, as originally reported.

With my patch, we won't issue any warnings, because I'm not sure if we can say
that in *any* instantiation of withMove the std::move is wrong.  Am I mistaken?

Thanks for your comments and sorry if I'm still not getting your point.

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[herring at lanl dot gov]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #7 from S. Davis Herring <herring at lanl dot gov> ---
> In the withMove case, in C++20, we issue:
> warning: moving a local object in a return statement prevents copy elision
> for
> template Dest withMove<Dest>();
> and:
> warning: redundant move in return statement
> for
> template Dest withMove<Source>();

Each of these in isolation is of course correct (which was of course never in
question).

> With my patch, we won't issue any warnings, because I'm not sure if we
> can say that in *any* instantiation of withMove the std::move is
> wrong.  Am I mistaken?

P1825R0 makes it much less likely that removing the std::move is ever a
problem.  There are cases that still need it, like

  struct A {
    operator Dest() &&;
  };
  template Dest withMove<A>();

That said, P2266R3 makes even that case work without std::move in C++23, and
GCC and Clang already accept it without std::move in C++20 mode (even though
it's not listed as a defect report).  I think that in these modes it's
impossible to need the std::move, so it's reasonable to issue the warning,
although it might make the most sense to issue a single, generic warning like
"std::move is either useless or harmful here depending on instantiation".

> Thanks for your comments and sorry if I'm still not getting your point.

Thank you for trying to work past my first unclear explanation.

[Bug c++/89780] -Wpessimizing-move is too agressive with templates and recommends pessimization

[herring at lanl dot gov]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780

--- Comment #8 from S. Davis Herring <herring at lanl dot gov> ---
I looked at P2266R3 again; it claims that the conversion function case (in #7)
is actually covered by P1825R0.  I think that case is questionable, since it
still refers to "overload resolution to select the constructor for the copy"
and there's no constructor involved when a conversion function is used.  (In
particular, it isn't the case that the A object is converted to a Dest
temporary that is then used as the argument to Dest(Dest&&), since that would
be a second user-defined conversion.)  If we consider the intent in P1155R3,
though, it's pretty clear that that's just a wording oversight, so it's not
unreasonable that GCC and Clang (but not ICC) accept

  template Dest withMove<A>();

in C++20 mode.  That means that we don't have to distinguish C++20 and C++23
for this discussion (at least outside of weirder cases like returning a
reference).