Warns when a function accepting a forwarding reference does anything besides
forwarding (with std::forward) the parameter in the body of the function.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
maybe some other name for check, like missing-std-forward.
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp | ||
---|---|---|
62 ↗ | (On Diff #508432) | move this up to be checkered first |
67 ↗ | (On Diff #508432) | anyway invert this logic. |
69–72 ↗ | (On Diff #508432) | use isDefinition also for functionDecl, hasAncestor(functionnDecl(isDefinition(), ToParam, unless(hasDescendant(ForwardCallMatcher))))))) I don't see reason for eplicit handling of cxxConstructorDecl, hasDescendant will match also initialization list. template<typename T> void test(T&& t) { using Type = decltype(callSomething(std::forward<T>(t))); callOther<Type>(t); } |
83–84 ↗ | (On Diff #508432) | I thing this can never happen |
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h | ||
32 ↗ | (On Diff #508432) | add here that thing about UnlessSpelledInSource.... |
- add tests, simplify expr, handle unevaluated exprs
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp | ||
---|---|---|
83–84 ↗ | (On Diff #508432) | Another review suggested I check the matched node for nullness, even though it was the only possible match (https://reviews.llvm.org/D140793). I'll keep it as is since in the spirit of defensive programming and to assist future developers who may add more matches that might invalidate the invariant that Param is always non-null. |
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h | ||
32 ↗ | (On Diff #508432) | Sorry, I think I'm missing some context here. Would you mind clarifying your comment? |
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
139 | Please follow 80 characters limit for text. |
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp | ||
---|---|---|
20 ↗ | (On Diff #508450) | move this matcher to some utils/... |
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h | ||
32 ↗ | (On Diff #508432) | I mean: std::optional<TraversalKind> getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; } just to specify this explicitly.. |
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst | ||
7 ↗ | (On Diff #508450) | Add link to cpp guidelines, mention rule name like in other checks. |
clang-tools-extra/docs/clang-tidy/checks/list.rst | ||
190 | no fixes | |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp | ||
138 ↗ | (On Diff #508450) | what about when someone uses std::move instead of std::format ? |
- fix docs, fix test
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp | ||
---|---|---|
20 ↗ | (On Diff #508450) | Will be done in https://reviews.llvm.org/D146929 |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp | ||
138 ↗ | (On Diff #508450) | Are you suggesting to have the tool add a special note in something like template <class T> void foo(T&& t) { T other = std::move(t); } I'm not sure I completely followed what you were saying. Or perhaps a fixit for this specific case of using move on a forwarding reference (fixit to replace move with forward). |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp | ||
---|---|---|
138 ↗ | (On Diff #508450) | Yes, I meant that situation. But only because "forwarding reference parameter 't' is never forwarded inside the function body" could be confused in such case, where there is std::move instead of std::forward. |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp | ||
---|---|---|
138 ↗ | (On Diff #508450) | Ya, https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html should cover this. This new check missing-std-forward only knows how to find code that is missing forward |
Overall looks fine. My main concern are lambdas (and maybe functions/classes in functions, but that should only hit performance).
Please close all comments before pushing this.
clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp | ||
---|---|---|
62 | probably auto would be sufficient. | |
71 | maybe think like: "functionDecl(forEachDescendant(parmVarDecl" could work. | |
72 | probably this could be named like isTemplateTypeParameter, current name suggest more that it's a FunctionDecl matcher, not ParmVarDecl. | |
72 | consider excluding system code, or code inside std namespace. | |
73 | maybe we should skip also template instances. | |
74 | consider std::move those matchers, always check construction could be faster... | |
clang-tools-extra/docs/clang-tidy/checks/list.rst | ||
192 | no fixes provided, remove | |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp | ||
138 | Add test with std::forward happen in lambda, like: template <class T> void does_something(T&& t) { [=] { call_other(std::forward<T>(t)); } } this may not be detected. auto ForwardCallMatcher = callExpr(forCallable(equalsBoundNode("first"))); ... hasAncestor(functionDecl().bind("first")), hasAncestor(functionDecl(isDefinition(), equalsBoundNode("first"), ToParam, unless(hasDescendant(ForwardCallMatcher))))), Problem with hasAncestor, is that if first ancestor does not match then it's trying parent one. equalsBoundNode can be used to limit this only to first hasAncestor of specific type (unless there is other matcher for that). |
clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp | ||
---|---|---|
71 | Not sure if I see any advantages of forEachDescendant over what I have here - I think I'll leave it as is. | |
72 | Why specifically do this in this check, but not others (very few seem to do this)? | |
73 | Not sure I follow. Can you give an example? |
Note, @PiotrZSL I don't have commit access, so if you're happy with the updates, could you please land this for me?
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
136 | Should be after cppcoreguidelines-misleading-capture-default-by-value. |
forwarding reference parameter '' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
consider ignoring unnamed parameters, they unused, so nothing to forward.
probably auto would be sufficient.