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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
