[PATCH] D11946: Create clang-tidy module modernize. Add pass-by-value check.
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 12 06:11:38 PDT 2015
alexfh added a comment.
Something weird happened with the file names in this CL after the latest update, e.g. there are both `clang-tidy/CMakeLists.txt` and `CMakeLists.txt`. How exactly did you create the Differential revision and update it?
================
Comment at: ModernizeTidyModule.cpp:13
@@ +12,3 @@
+#include "../ClangTidyModuleRegistry.h"
+#include "../readability/BracesAroundStatementsCheck.h"
+#include "../readability/FunctionSizeCheck.h"
----------------
Looks like you've forgotten to remove the `../readability` includes after copy-pasting.
================
Comment at: ModernizeTidyModule.cpp:31
@@ +30,3 @@
+
+ ClangTidyOptions getModuleOptions() override {
+ ClangTidyOptions Options;
----------------
Just don't override this method, if you don't add any module options.
================
Comment at: PassByValueCheck.cpp:26
@@ +25,3 @@
+
+/// \brief Matches move constructible classes.
+///
----------------
`move-constructible` seems slightly more common than `move constructible`, but I'm not a native speaker.
================
Comment at: PassByValueCheck.cpp:30
@@ +29,3 @@
+/// \code
+/// // POD types are trivially move constructible
+/// struct Foo { int a; };
----------------
nit: Missing trailing period.
================
Comment at: PassByValueCheck.cpp:82
@@ +81,3 @@
+ /// given constructor.
+ bool hasExactlyOneUsageIn(const CXXConstructorDecl *Ctor) {
+ Count = 0;
----------------
This method seems to be the only useful interface of this class. How about making this method a free-standing function and make this class an implementation detail of it (e.g. by defining it inside the function)?
================
Comment at: PassByValueCheck.cpp:93
@@ +92,3 @@
+ bool VisitDeclRefExpr(DeclRefExpr *D) {
+ if (const ParmVarDecl *To = dyn_cast<ParmVarDecl>(D->getDecl()))
+ if (To == ParamDecl) {
----------------
nit: I'd put the body of this conditional statement in braces as it spans more than one line.
================
Comment at: PassByValueCheck.cpp:93
@@ +92,3 @@
+ bool VisitDeclRefExpr(DeclRefExpr *D) {
+ if (const ParmVarDecl *To = dyn_cast<ParmVarDecl>(D->getDecl()))
+ if (To == ParamDecl) {
----------------
alexfh wrote:
> nit: I'd put the body of this conditional statement in braces as it spans more than one line.
nit: Since the `To` variable is only used once, why not just compare `dyn_cast<...>(...)` with `ParamDecl`?
================
Comment at: PassByValueCheck.cpp:96
@@ +95,3 @@
+ ++Count;
+ if (Count > 1)
+ // no need to look further, used more than once
----------------
nit: I'd put the body of this conditional statement in braces as it spans more than one line.
================
Comment at: PassByValueCheck.cpp:97
@@ +96,3 @@
+ if (Count > 1)
+ // no need to look further, used more than once
+ return false;
----------------
Please use proper Capitalization and punctuation (namely, trailing period).
================
Comment at: PassByValueCheck.cpp:112
@@ +111,3 @@
+ const ParmVarDecl *ParamDecl) {
+ ExactlyOneUsageVisitor Visitor(ParamDecl);
+ return Visitor.hasExactlyOneUsageIn(Ctor);
----------------
nit: Consider moving the class definition here since it's not used elsewhere.
Also, maybe `return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor);`?
================
Comment at: PassByValueCheck.cpp:118
@@ +117,3 @@
+/// redeclarations of \p Ctor.
+static void collectParamDecls(const CXXConstructorDecl *Ctor,
+ const ParmVarDecl *ParamDecl,
----------------
nit: Maybe return the result?
================
Comment at: PassByValueCheck.cpp:120
@@ +119,3 @@
+ const ParmVarDecl *ParamDecl,
+ SmallVectorImpl<const ParmVarDecl *> &Results) {
+ unsigned ParamIdx = ParamDecl->getFunctionScopeIndex();
----------------
Please use `ArrayRef` (passed by value) instead. It's a more generic way to pass a range of items to a function.
================
Comment at: PassByValueCheck.cpp:140
@@ +139,3 @@
+ hasType(qualType(
+ // match only constref or a non-const value
+ // parameters, rvalues and const-values
----------------
Capitalization. Punctuation.
================
Comment at: PassByValueCheck.cpp:161
@@ +160,3 @@
+void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
+ const CXXConstructorDecl *Ctor =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("Ctor");
----------------
RHS already spells the type, use `const auto *Ctor`.
================
Comment at: PassByValueCheck.cpp:163
@@ +162,3 @@
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("Ctor");
+ const ParmVarDecl *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>("Param");
+ const CXXCtorInitializer *Initializer =
----------------
ditto
================
Comment at: PassByValueCheck.cpp:164
@@ +163,3 @@
+ const ParmVarDecl *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>("Param");
+ const CXXCtorInitializer *Initializer =
+ Result.Nodes.getNodeAs<CXXCtorInitializer>("Initializer");
----------------
ditto
================
Comment at: PassByValueCheck.cpp:176
@@ +175,3 @@
+
+ auto Diag = diag(ParamDecl->getLocStart(), "Pass by value and use std::move");
+
----------------
Diagnostic messages should start with a lower-case letter.
================
Comment at: PassByValueCheck.cpp:181
@@ +180,3 @@
+ TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+ ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
+
----------------
Use `auto`.
================
Comment at: PassByValueCheck.cpp:190
@@ +189,3 @@
+ ParamTL.getLocEnd());
+ auto ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
+ ValueTL.getSourceRange()),
----------------
Here it's not immediately obvious what type is `ValueStr`. Please use the specific type here.
================
Comment at: PassByValueCheck.cpp:198
@@ +197,3 @@
+
+ // Move the value in the initialization list.
+ Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")")
----------------
nit: Move the value _to_ the ...
================
Comment at: test/clang-tidy/modernize-pass-by-value.cpp:4
@@ +3,3 @@
+
+// CHECK: #include <utility>
+
----------------
Does this work for you? If yes, I'm confused. The script should only understand `CHECK-MESSAGES` and `CHECK-FIXES` prefixes.
================
Comment at: test/clang-tidy/modernize-pass-by-value.cpp:7
@@ +6,3 @@
+namespace {
+// POD types are trivially move constructible
+struct Movable {
----------------
nit: Trailing period.
================
Comment at: test/clang-tidy/modernize-pass-by-value.cpp:23
@@ +22,3 @@
+ // CHECK: A(Movable M) : M(std::move(M)) {}
+ // SAFE_RISK: A(const Movable &M) : M(M) {}
+ Movable M;
----------------
What's that?
http://reviews.llvm.org/D11946
More information about the cfe-commits
mailing list