Coding style and warning spam: redundant std::move?

Hi all,

GCC 9 introduced a new warning, -Wredundant-move, which is enabled by
default when building LLVM and produces what looks like at least
thousands of hits.

https://reviews.llvm.org/D74672 is a sample of the kind of changes
pointed out by this warning, more explanations are in this blog post:
https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c/

What do people think should be done here?

1. Disable -Wredundant-move?
2. Fix it all (seems daunting to do manually)?
3. Encourage clang/clang-tidy developers to add this warning and add a
clang-tidy rule to fix it automatically?

My personal opinion is that while the review linked above should be
committed, it's ultimately a drop in the ocean and #3 is the way to
go.

In the meantime, I'm certainly going to disable -Wredundant-move
locally, but should that also be done by default for gcc in the
CMakeLists.txt?

Cheers,
Nicolai

Hi all,

GCC 9 introduced a new warning, -Wredundant-move, which is enabled by
default when building LLVM and produces what looks like at least
thousands of hits.

https://reviews.llvm.org/D74672 is a sample of the kind of changes
pointed out by this warning, more explanations are in this blog post:
https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c/

What do people think should be done here?

  1. Disable -Wredundant-move?
  2. Fix it all (seems daunting to do manually)?
  3. Encourage clang/clang-tidy developers to add this warning and add a
    clang-tidy rule to fix it automatically?

My personal opinion is that while the review linked above should be
committed, it’s ultimately a drop in the ocean and #3 is the way to
go.

+1

In the meantime, I’m certainly going to disable -Wredundant-move
locally, but should that also be done by default for gcc in the
CMakeLists.txt?

I’d support this as well!

Thanks!

You cannot really fix the issue until the minimum required GCC version for the project is bumped because changing “std::move(Err)” to just “Err” may (and most likely will) result in compile errors. The following case:

struct Something {};

struct CreateFromMoveable {
CreateFromMoveable(T&&);
};

CreateFromMoveable f() {
Something S;
// …
return S;
}

is a hard failure with old GCC, that’s why “std::move” is written there. Once the “std::move” is there, old GCC properly picks up the fact that it could bind the move ctor in the return.

For now, you could either fall back to using an older compiler or disable the warning locally…

Nicolai Hähnle via cfe-dev <cfe-dev@lists.llvm.org> ezt írta (időpont: 2020. febr. 15., Szo, 20:35):

Seems like this was fixed in gcc5?

https://godbolt.org/z/zaDyt6

Hi Mehdi, Nicolai,

As the author of P1155, I probably know the most about the tangle that compilers (and WG21) have made of the situation. :slight_smile:

GCC’s -Wredundant-move is a super useful warning for people who build only with GCC trunk and want to migrate off of std::move as quickly as possible. It is not appropriate for codebases that need to compile with other present-day compilers (e.g. Clang).

For example, here is a case (isomorphic to some of the JSON-related cases in the above patch, AIUI) where GCC’s new warning recommends to remove std::move, but if you do that, you’ll get silently worse codegen on Clang — that is, if you remove std::move, the code no longer moves, it does a copy instead.
https://godbolt.org/z/Ye9Lnt

And here is a case where GCC’s new warning recommends to remove std::move, but if you do that, the code stops building on Clang entirely:
https://godbolt.org/z/sgrBkc

So for the foreseeable future, I would strongly recommend that Nicolai just pass -Wno-redundant-move when building with GCC.

What would be super useful, IMO, would be if Nicolai (or anyone) would look into how to bring Clang into line with GCC (or even all the way into line with the brand-new C++20 standard, which makes several massive changes in this area), so that return x would hardly ever make a copy of x.

I did some work in that area a year or two ago, but my work on -Wreturn-std-move was actually aimed at telling people to add std::move to their returns, in cases such as the above snippets where they were silently getting copies and probably didn’t expect copies. See https://wg21.link/p1155 for the field experience report.

HTH,
–Arthur

I apologise, I wrote the example out of my mind without exact details - I just remembered that there are issues with adding/removing move when you want to target multiple compilers. It could be, that in the GCC line it works, but older Clangs don’t support it, maybe you need two layers of template instantiation or return value wrapping for this to break. It’s been a while since I had the exact case in front of me.

Mehdi AMINI <joker.eph@gmail.com> ezt írta (időpont: 2020. febr. 15., Szo, 21:42):

It’s worth noting that somebody tried to remove many supposedly-redundant std::move calls (see https://github.com/llvm/llvm-project/commit/1c2241a7936bf85aa68aef94bd40c3ba77d8ddf2#diff-7046c45834a9a01f5ea1571c64d347bb), but this broke most or all the build bots, presumably because of reasons as outlined, so was reverted.