This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement cppcoreguidelines F.19
ClosedPublic

Authored by ccotter on Mar 26 2023, 10:29 AM.

Details

Summary

Warns when a function accepting a forwarding reference does anything besides
forwarding (with std::forward) the parameter in the body of the function.

Diff Detail

Event Timeline

ccotter created this revision.Mar 26 2023, 10:29 AM
ccotter requested review of this revision.Mar 26 2023, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2023, 10:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ccotter retitled this revision from Reviewers: to [clang-tidy] Implement cppcoreguidelines F.19 .Mar 26 2023, 10:29 AM
ccotter edited the summary of this revision. (Show Details)

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.
would be nice to invert this to start matching with functionDecl, but we don't have forEachParameter

69–72 ↗(On Diff #508432)

use isDefinition also for functionDecl,
anyway maybe

hasAncestor(functionnDecl(isDefinition(),
                                            ToParam,
                                            unless(hasDescendant(ForwardCallMatcher)))))))

I don't see reason for eplicit handling of cxxConstructorDecl, hasDescendant will match also initialization list.
Exclude code that is used in decltype, like:

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

ccotter updated this revision to Diff 508441.Mar 26 2023, 2:20 PM
ccotter marked 4 inline comments as done.
ccotter retitled this revision from [clang-tidy] Implement cppcoreguidelines F.19 to [clang-tidy] Implement cppcoreguidelines F.19.
  • 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?

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
139

Please follow 80 characters limit for text.

ccotter updated this revision to Diff 508450.Mar 26 2023, 5:19 PM
  • add tests, simplify expr, handle unevaluated exprs
  • formatting
PiotrZSL added inline comments.Mar 26 2023, 11:37 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp
20 ↗(On Diff #508450)

move this matcher to some utils/...
It may be needed by other checks.

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 ?
maybe some "note" for such issue ?

ccotter updated this revision to Diff 508869.Mar 27 2023, 7:08 PM
ccotter marked 2 inline comments as done.
  • fix docs, fix test
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp
20 ↗(On Diff #508450)
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).

ccotter updated this revision to Diff 509539.Mar 29 2023, 8:23 PM
  • Use common function
ccotter marked 4 inline comments as done.Mar 29 2023, 8:24 PM
ccotter updated this revision to Diff 511847.Apr 7 2023, 9:50 PM
  • Rename
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp
138 ↗(On Diff #508450)

@PiotrZSL - I wasn't sure what you meant here.

PiotrZSL added inline comments.Apr 10 2023, 11:23 PM
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.
But that could be ignored, or covered by other check that would simply detect that you doing move when you should do forward.

ccotter marked 2 inline comments as done.Apr 11 2023, 7:41 AM
ccotter added inline comments.
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

ccotter updated this revision to Diff 512455.Apr 11 2023, 7:43 AM
ccotter marked an inline comment as done.

Fix tests

PiotrZSL accepted this revision.May 2 2023, 1:49 PM

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.
I usually solve those things like this:

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

This revision is now accepted and ready to land.May 2 2023, 1:49 PM
PiotrZSL set the repository for this revision to rG LLVM Github Monorepo.May 4 2023, 10:51 AM
ccotter updated this revision to Diff 520026.May 5 2023, 8:28 PM
ccotter marked 6 inline comments as done.
  • feedback
ccotter added inline comments.May 5 2023, 8:30 PM
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?

ccotter updated this revision to Diff 520032.May 5 2023, 9:51 PM
  • fix merge
Eugene.Zelenko added inline comments.May 5 2023, 10:07 PM
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.

PiotrZSL updated this revision to Diff 520043.May 6 2023, 1:35 AM

Add delayed template parsing in tests (to fix windows tests)
Reorder release notes

This revision was automatically updated to reflect the committed changes.