Fix bugprone-move-forwarding-reference violations #551
Conversation
facebook-github-bot
added
the
CLA Signed
include/unifex/v2/async_scope.hpp
Outdated
| } | ||
| }); | ||
| } | ||
|
|
||
| template <typename E> | ||
| void set_error(E e) noexcept { | ||
| complete([&](auto&& receiver) noexcept { | ||
| unifex::set_error(std::move(receiver), std::move(e)); | ||
| unifex::set_error(std::forward<decltype(receiver)>(receiver), std::move(e)); |
There was a problem hiding this comment.
This is semi debatable. In the letter of the law (the law being CppCoreGuidelines F.19 checked by bugprone-move-forwarding-reference, this should use a forward. I think receivers usually or always are passed by rvalue reference within the set_error CPO, but if someone somehow ends up trying to pass an lvalue ref to a receiver that does or doesn't accept by lvalue ref on the receiver, this code is not taking an opinion on way or the other - the bug would be elsewhere.
There was a problem hiding this comment.
complete is defined below, on line 285, and it always passes the operation's Receiver by rvalue reference. This auto&& is just saving characters. Perhaps a better way to silence the linter here is to spend the characters and type out Receiver&&.
|
Note, I chose to use |
There was a problem hiding this comment.
Comparing the build times on this PR's CI to those on #552, it looks like switching to std::forward<>() isn't very expensive; comparable runs vary by about +/- 30 seconds in both directions. We may as well switch—std::forward<>() conveys intent better than some arbitrary static_cast.
Please just update the lambdas passed to complete not to take auto&& so we can stick with std::move() instead of silencing the linter with an over-general std::forward<>().
include/unifex/v2/async_scope.hpp
Outdated
| } | ||
| }); | ||
| } | ||
|
|
||
| template <typename E> | ||
| void set_error(E e) noexcept { | ||
| complete([&](auto&& receiver) noexcept { | ||
| unifex::set_error(std::move(receiver), std::move(e)); | ||
| unifex::set_error(std::forward<decltype(receiver)>(receiver), std::move(e)); |
There was a problem hiding this comment.
complete is defined below, on line 285, and it always passes the operation's Receiver by rvalue reference. This auto&& is just saving characters. Perhaps a better way to silence the linter here is to spend the characters and type out Receiver&&.
|
I can't figure out how to squash this nicely from my phone. I'll merge it once the kids are in bed. |
No description provided.