[PATCH] D141569: [clang-tidy] Implement CppCoreGuideline F.18

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Overall not bad, except reported things, I don't have any complains.
Number of options is not an issue.
90% of users wont use them, 10% will be happy to find them instead of dealing with NOLINT.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:51
+              lambdaExpr(hasDescendant(declRefExpr(equalsBoundNode("ref"))),
+                         valueCapturesVar(equalsBoundNode("param"))))))
+          .bind("move-call");
----------------
i thing you dont need here "hasDescendant(declRefExpr(equalsBoundNode("ref"))),"
simply because hasAncestor alone is doing a trick, if lambda is ancestor, then this call is a descendant for it.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:56-57
+      parmVarDecl(
+          parmVarDecl(hasType(type(rValueReferenceType()))).bind("param"),
+          parmVarDecl(
+              optionally(hasType(
----------------
consider:
```
hasType(type(rValueReferenceType())),
decl().bind("param"),
optionally(hasType(
                  qualType(references(templateTypeParmType(hasDeclaration(
                      templateTypeParmDecl().bind("template-type"))))))),
```
no need to duplicate parmVarDecl


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:61
+                      templateTypeParmDecl().bind("template-type"))))))),
+              unless(hasType(references(qualType(
+                  anyOf(isConstQualified(), substTemplateTypeParmType()))))),
----------------
put this unless before optionally...


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:64-74
+                  hasAncestor(compoundStmt(hasParent(lambdaExpr(
+                      has(cxxRecordDecl(
+                          has(cxxMethodDecl(ToParam, hasName("operator()"))))),
+                      optionally(hasDescendant(MoveCallMatcher)))))),
+                  hasAncestor(cxxConstructorDecl(
+                      ToParam, isDefinition(), unless(isMoveConstructor()),
+                      optionally(hasDescendant(MoveCallMatcher)))),
----------------
looks like lambda context is visible as both operator() and as body to lambaExpr directly.
this mean that it may match twice, once as functionDecl, and once as lambdaExpr.
You can merge functionDecl with cxxConstructorDecl, just do same trick like with isMoveAssignmentOperator.

I would probably start with functionDecl, and then would try do some trick like forEachParam, but we dont have such matcher....
You missing also isDefinition() in functionDecl even that you got hasBody.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:110
+  const auto *MoveCall = Result.Nodes.getNodeAs<CallExpr>("move-call");
+  if (!MoveCall) {
+    diag(Param->getLocation(),
----------------
consider style:
if (MoveCall) return;

 


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst:24
+
+.. option:: AllowAnyMoveExpr
+
----------------
maybe AllowPartialMove


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141569