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