PiotrZSL added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:22-24
+ parmVarDecl(hasType(type(rValueReferenceType()))).bind("param"),
+ parmVarDecl(
+ equalsBoundNode("param"),
----------------
those lines should be equal to just:
parmVarDecl(hasType(type(rValueReferenceType()))).bind("param"),
i'm not sure why you need additional equalsBoundNode, thats useful mainly if
you would do some tricks with hasParent or something...
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:25-29
+ unless(hasType(references(isConstQualified()))),
+ unless(hasType(qualType(references(templateTypeParmType(
+ hasDeclaration(templateTypeParmDecl())))))),
+ unless(
+ hasType(qualType(references(substTemplateTypeParmType())))),
----------------
you could try to merge those 3:
```
unless(hasType(qualType(references(anyOf(isConstQualified(),
templateTypeParmType(hasDeclaration(templateTypeParmDecl())),
substTemplateTypeParmType())))
```
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:40
+ isDefinition(), ToParam,
+ unless(cxxConstructorDecl(isMoveConstructor())),
+ unless(cxxMethodDecl(isMoveAssignmentOperator())))
----------------
you already got constructor as second in any of, it shouldnt be matched here,
so this line shouldnt be needed
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:46
+
+static bool isValueCapturedByAnyLambda(ASTContext &Context,
+ const FunctionDecl *Function,
----------------
you dont need to split things like std::move or lambda detections.
you can add this right to matcher, like:
```
unless(hasDescendant(lambdaExpr(hasAnyCapture(capturesVar(equalsBoundMode("param"),
...
```
and here you may need to add new matcher to match ByCopy))
but you dont need to check lambdas in this way, you dont need to check them
anyway, they should be check as descendants of functions
```
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp:103-125
+ StatementMatcher MoveCallMatcher = callExpr(
+ anyOf(callee(functionDecl(hasName("::std::move"))),
+ callee(unresolvedLookupExpr(hasAnyDeclaration(
+ namedDecl(hasUnderlyingDecl(hasName("::std::move"))))))),
+ argumentCountIs(1), hasArgument(0, moveArgumentOf(StrictMode, Param)));
+
+ SmallVector<BoundNodes, 1> Matches;
----------------
```
unless(hasDescendant(callExpr(
anyOf(callee(functionDecl(hasName("::std::move"))),
callee(unresolvedLookupExpr(hasAnyDeclaration(
namedDecl(hasUnderlyingDecl(hasName("::std::move"))))))),
argumentCountIs(1), hasArgument(0, moveArgumentOf(StrictMode, Param)))))
```
any here you could also check lambdas, if you want to ignore copies, but not
should why you should do that, but you can always bind this to "move", or
implement lambda checking as matcher....
unless(hasAncestor(lambdaExpr(isCapturedByCopy....
Simply as you have some tests already, try pack as much as you can into
registerMatchers, because once you split this across multiple submatches in
diffrent functions, then it could be hard to maintain.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h:32-33
+private:
+ const unsigned StrictMode : 1;
+ const unsigned IgnoreUnnamedParams : 1;
+};
----------------
use bool here, or it could cause some issues, you wont save anything by using
bits. class alignment is already 8.
problem is that you read bools but in storeOptions you store ints, and when
dump-config will be used it will show 1/0 not true/false.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst:1
+.. title:: clang-tidy - cppcoreguidelines-rvalue-reference-param-not-moved
+
----------------
add explicit documentation about options
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst:13
+
+Examples:
+
----------------
one example
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
