Add missing std::forward calls #8049
Conversation
Found by enabling cppcoreguidelines-missing-std-forward (https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/missing-std-forward.html) -- that specific check isn't ready to be enabled for other reasons (it changes the behavior of other checks in ways that are painful) but doing this cleanup seems modestly worthwhile.
| @@ -930,17 +930,17 @@ std::ostream &operator<<(std::ostream &s, const BinOp<Mod, A, B> &op) { | |||
| } | |||
There was a problem hiding this comment.
IIUC std::forward is supposed to be strictly better. However, this whole file is very sensitive to what the compiler decides needs an address and stack location and what doesn't, so this change is not a no-brainer. I need to check what the stack frame looks like for some of the simplify visitors before and after this change.
There was a problem hiding this comment.
I don't consider this change really essential... basically just wanted to grind through the new checks while waiting on some buildbots. This can wait for a lazy day. (Or be merged piecewise.)
| @@ -2338,7 +2334,7 @@ class Buffer { | |||
| typename... Args, | |||
| typename = decltype(std::declval<Fn>()(std::declval<Args>()...))> | |||
| HALIDE_ALWAYS_INLINE static void for_each_element_variadic(int, int, const for_each_element_task_dim *, Fn &&f, Args... args) { | |||
| f(args...); | |||
| std::forward<Fn>(f)(args...); | |||
There was a problem hiding this comment.
This one's a bit weird. What does it mean to std::forward the this parameter of operator()?
|
Are you up for moving the small style fixes into another PR? It should be trivial to land and makes this one smaller. Optionally, update the description to explain the other changes. |
Yeah, looks like this has a lot of cruft in it that I didn't expect. Converting to draft for now. |
Found by enabling cppcoreguidelines-missing-std-forward (https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/missing-std-forward.html) -- that specific check isn't ready to be enabled for other reasons (it changes the behavior of other checks in ways that are painful) but doing this cleanup seems modestly worthwhile.