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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
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
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Coding style and warning spam: redundant std::move?

suyash singh via cfe-dev


On Sat, Feb 15, 2020 at 11:35 AM Nicolai Hähnle via llvm-dev <[hidden email]> wrote:
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!

-- 
Mehdi 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
In reply to this post by suyash singh via cfe-dev
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 {};
  <T> struct CreateFromMoveable {
    CreateFromMoveable(T&&);
  };

  CreateFromMoveable<Something> 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 <[hidden email]> ezt írta (időpont: 2020. febr. 15., Szo, 20:35):
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
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev


On Sat, Feb 15, 2020 at 12:13 PM Whisperity via cfe-dev <[hidden email]> wrote:
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 {};
  <T> struct CreateFromMoveable {
    CreateFromMoveable(T&&);
  };

  CreateFromMoveable<Something> 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.

Seems like this was fixed in gcc5?


-- 
Mehdi

 

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


Nicolai Hähnle via cfe-dev <[hidden email]> ezt írta (időpont: 2020. febr. 15., Szo, 20:35):
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
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
On Sat, Feb 15, 2020 at 3:42 PM Mehdi AMINI via cfe-dev <[hidden email]> wrote:
On Sat, Feb 15, 2020 at 12:13 PM Whisperity via cfe-dev <[hidden email]> wrote:
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 {};
  <T> struct CreateFromMoveable {
    CreateFromMoveable(T&&);
  };

  CreateFromMoveable<Something> 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.

Seems like this was fixed in gcc5?


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. :)

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.

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:

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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
In reply to this post by suyash singh via cfe-dev
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 <[hidden email]> ezt írta (időpont: 2020. febr. 15., Szo, 21:42):


On Sat, Feb 15, 2020 at 12:13 PM Whisperity via cfe-dev <[hidden email]> wrote:
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 {};
  <T> struct CreateFromMoveable {
    CreateFromMoveable(T&&);
  };

  CreateFromMoveable<Something> 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.

Seems like this was fixed in gcc5?


-- 
Mehdi

 

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


Nicolai Hähnle via cfe-dev <[hidden email]> ezt írta (időpont: 2020. febr. 15., Szo, 20:35):
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
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

suyash singh via cfe-dev
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.

On Sun, 16 Feb 2020 at 14:07, Whisperity via cfe-dev <[hidden email]> wrote:
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 <[hidden email]> ezt írta (időpont: 2020. febr. 15., Szo, 21:42):


On Sat, Feb 15, 2020 at 12:13 PM Whisperity via cfe-dev <[hidden email]> wrote:
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 {};
  <T> struct CreateFromMoveable {
    CreateFromMoveable(T&&);
  };

  CreateFromMoveable<Something> 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.

Seems like this was fixed in gcc5?


-- 
Mehdi

 

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


Nicolai Hähnle via cfe-dev <[hidden email]> ezt írta (időpont: 2020. febr. 15., Szo, 20:35):
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
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev