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

[Chris Cotter via Phabricator via cfe-commits]

ccotter added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:70
+  Finder->addMatcher(
+      parmVarDecl(
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
----------------
PiotrZSL wrote:
> maybe think like: "functionDecl(forEachDescendant(parmVarDecl" could work.
Not sure if I see any advantages of `forEachDescendant` over what I have here - I think I'll leave it as is.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:71
+      parmVarDecl(
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
+          hasAncestor(functionDecl(isDefinition(), ToParam,
----------------
PiotrZSL wrote:
> 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.
Why specifically do this in this check, but not others (very few seem to do this)?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp:72
+          parmVarDecl().bind("param"), isTemplateTypeOfFunction(),
+          hasAncestor(functionDecl(isDefinition(), ToParam,
+                                   unless(hasDescendant(ForwardCallMatcher))))),
----------------
PiotrZSL wrote:
> maybe we should skip also template instances.
Not sure I follow. Can you give an example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146921