[PATCH] D146921: [clang-tidy] Implement cppcoreguidelines F.19

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL accepted this revision.
PiotrZSL added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:61
+
+  StatementMatcher ForwardCallMatcher = callExpr(
+      argumentCountIs(1),
----------------
probably `auto` would be sufficient.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:70
+  Finder->addMatcher(
+      parmVarDecl(
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
----------------
maybe think like: "functionDecl(forEachDescendant(parmVarDecl" could work.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:71
+      parmVarDecl(
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
+          hasAncestor(functionDecl(isDefinition(), ToParam,
----------------
probably this could be named like isTemplateTypeParameter, current name suggest more that it's a FunctionDecl matcher, not ParmVarDecl.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:71
+      parmVarDecl(
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
+          hasAncestor(functionDecl(isDefinition(), ToParam,
----------------
PiotrZSL wrote:
> probably this could be named like isTemplateTypeParameter, current name suggest more that it's a FunctionDecl matcher, not ParmVarDecl.
consider excluding system code, or code inside std namespace.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:72
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
+          hasAncestor(functionDecl(isDefinition(), ToParam,
+                                   unless(hasDescendant(ForwardCallMatcher))))),
----------------
maybe we should skip also template instances.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:73
+          hasAncestor(functionDecl(isDefinition(), ToParam,
+                                   unless(hasDescendant(ForwardCallMatcher))))),
+      this);
----------------
consider `std::move` those matchers, always check construction could be faster...


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:192
    `cppcoreguidelines-macro-usage <cppcoreguidelines/macro-usage.html>`_,
+   `cppcoreguidelines-missing-std-forward <cppcoreguidelines/missing-std-forward.html>`_, "Yes"
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines/narrowing-conversions.html>`_,
----------------
no fixes provided, remove


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp:137
+};
+
+} // namespace negative_cases
----------------
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146921/new/

https://reviews.llvm.org/D146921