angelgarcia updated this revision to Diff 32036. angelgarcia marked 4 inline comments as done. angelgarcia added a comment.
Set IncludeStyle as an option. Fix comments. http://reviews.llvm.org/D11946 Files: clang-tidy/CMakeLists.txt clang-tidy/Makefile clang-tidy/modernize/CMakeLists.txt clang-tidy/modernize/Makefile clang-tidy/modernize/ModernizeTidyModule.cpp clang-tidy/modernize/PassByValueCheck.cpp clang-tidy/modernize/PassByValueCheck.h clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp clang-tidy/tool/Makefile test/clang-tidy/modernize-pass-by-value.cpp
Index: test/clang-tidy/modernize-pass-by-value.cpp =================================================================== --- test/clang-tidy/modernize-pass-by-value.cpp +++ test/clang-tidy/modernize-pass-by-value.cpp @@ -0,0 +1,197 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-pass-by-value %t +// REQUIRES: shell + +// CHECK-FIXES: #include <utility> + +namespace { +// POD types are trivially move constructible. +struct Movable { + int a, b, c; +}; + +struct NotMovable { + NotMovable() = default; + NotMovable(const NotMovable &) = default; + NotMovable(NotMovable &&) = delete; + int a, b, c; +}; +} + +struct A { + A(const Movable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: A(Movable M) : M(std::move(M)) {} + Movable M; +}; + +// Test that we aren't modifying other things than a parameter. +Movable GlobalObj; +struct B { + B(const Movable &M) : M(GlobalObj) {} + // CHECK-FIXES: B(const Movable &M) : M(GlobalObj) {} + Movable M; +}; + +// Test that a parameter with more than one reference to it won't be changed. +struct C { + // Tests extra-reference in body. + C(const Movable &M) : M(M) { this->i = M.a; } + // CHECK-FIXES: C(const Movable &M) : M(M) { this->i = M.a; } + + // Tests extra-reference in init-list. + C(const Movable &M, int) : M(M), i(M.a) {} + // CHECK-FIXES: C(const Movable &M, int) : M(M), i(M.a) {} + Movable M; + int i; +}; + +// Test that both declaration and definition are updated. +struct D { + D(const Movable &M); + // CHECK-FIXES: D(Movable M); + Movable M; +}; +D::D(const Movable &M) : M(M) {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move +// CHECK-FIXES: D::D(Movable M) : M(std::move(M)) {} + +// Test with default parameter. +struct E { + E(const Movable &M = Movable()) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: E(Movable M = Movable()) : M(std::move(M)) {} + Movable M; +}; + +// Test with object that can't be moved. +struct F { + F(const NotMovable &NM) : NM(NM) {} + // CHECK-FIXES: F(const NotMovable &NM) : NM(NM) {} + NotMovable NM; +}; + +// Test unnamed parameter in declaration. +struct G { + G(const Movable &); + // CHECK-FIXES: G(Movable ); + Movable M; +}; +G::G(const Movable &M) : M(M) {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move +// CHECK-FIXES: G::G(Movable M) : M(std::move(M)) {} + +// Test parameter with and without qualifier. +namespace ns_H { +typedef ::Movable HMovable; +} +struct H { + H(const ns_H::HMovable &M); + // CHECK-FIXES: H(ns_H::HMovable M); + ns_H::HMovable M; +}; +using namespace ns_H; +H::H(const HMovable &M) : M(M) {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move +// CHECK-FIXES: H(HMovable M) : M(std::move(M)) {} + +// Try messing up with macros. +#define MOVABLE_PARAM(Name) const Movable & Name +// CHECK-FIXES: #define MOVABLE_PARAM(Name) const Movable & Name +struct I { + I(MOVABLE_PARAM(M)) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: I(MOVABLE_PARAM(M)) : M(M) {} + Movable M; +}; +#undef MOVABLE_PARAM + +// Test that templates aren't modified. +template <typename T> struct J { + J(const T &M) : M(M) {} + // CHECK-FIXES: J(const T &M) : M(M) {} + T M; +}; +J<Movable> j1(Movable()); +J<NotMovable> j2(NotMovable()); + +struct K_Movable { + K_Movable() = default; + K_Movable(const K_Movable &) = default; + K_Movable(K_Movable &&o) { dummy = o.dummy; } + int dummy; +}; + +// Test with movable type with an user defined move constructor. +struct K { + K(const K_Movable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: K(K_Movable M) : M(std::move(M)) {} + K_Movable M; +}; + +template <typename T> struct L { + L(const Movable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: L(Movable M) : M(std::move(M)) {} + Movable M; +}; +L<int> l(Movable()); + +// Test with a non-instantiated template class. +template <typename T> struct N { + N(const Movable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: N(Movable M) : M(std::move(M)) {} + + Movable M; + T A; +}; + +// Test with value parameter. +struct O { + O(Movable M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: O(Movable M) : M(std::move(M)) {} + Movable M; +}; + +// Test with a const-value parameter. +struct P { + P(const Movable M) : M(M) {} + // CHECK-FIXES: P(const Movable M) : M(M) {} + Movable M; +}; + +// Test with multiples parameters where some need to be changed and some don't. +// need to. +struct Q { + Q(const Movable &A, const Movable &B, const Movable &C, double D) + : A(A), B(B), C(C), D(D) {} + // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: pass by value and use std::move + // CHECK-MESSAGES: :[[@LINE-3]]:41: warning: pass by value and use std::move + // CHECK-FIXES: Q(const Movable &A, Movable B, Movable C, double D) + // CHECK-FIXES: : A(A), B(std::move(B)), C(std::move(C)), D(D) {} + const Movable &A; + Movable B; + Movable C; + double D; +}; + +// Test that value-parameters with a nested name specifier are left as-is. +namespace ns_R { +typedef ::Movable RMovable; +} +struct R { + R(ns_R::RMovable M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move + // CHECK-FIXES: R(ns_R::RMovable M) : M(std::move(M)) {} + ns_R::RMovable M; +}; + +// Test with rvalue parameter. +struct S { + S(Movable &&M) : M(M) {} + // CHECK-FIXES: S(Movable &&M) : M(M) {} + Movable M; +}; + Index: clang-tidy/tool/Makefile =================================================================== --- clang-tidy/tool/Makefile +++ clang-tidy/tool/Makefile @@ -17,9 +17,9 @@ include $(CLANG_LEVEL)/../../Makefile.config LINK_COMPONENTS := $(TARGETS_TO_BUILD) asmparser bitreader support mc option USEDLIBS = clangTidy.a clangTidyLLVMModule.a clangTidyGoogleModule.a \ - clangTidyMiscModule.a clangTidyReadability.a clangTidyUtils.a \ - clangStaticAnalyzerFrontend.a clangStaticAnalyzerCheckers.a \ - clangStaticAnalyzerCore.a \ + clangTidyMiscModule.a clangTidyModernizeModule.a clangTidyReadability.a \ + clangTidyUtils.a clangStaticAnalyzerFrontend.a \ + clangStaticAnalyzerCheckers.a clangStaticAnalyzerCore.a \ clangFormat.a clangASTMatchers.a clangTooling.a clangToolingCore.a \ clangFrontend.a clangSerialization.a clangDriver.a clangParse.a \ clangSema.a clangAnalysis.a clangRewriteFrontend.a clangRewrite.a \ Index: clang-tidy/tool/ClangTidyMain.cpp =================================================================== --- clang-tidy/tool/ClangTidyMain.cpp +++ clang-tidy/tool/ClangTidyMain.cpp @@ -346,6 +346,10 @@ extern volatile int MiscModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED MiscModuleAnchorDestination = MiscModuleAnchorSource; +// This anchor is used to force the linker to link the ModernizeModule. +extern volatile int ModernizeModuleAnchorSource; +static int ModernizeModuleAnchorDestination = ModernizeModuleAnchorSource; + // This anchor is used to force the linker to link the ReadabilityModule. extern volatile int ReadabilityModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination = ReadabilityModuleAnchorSource; Index: clang-tidy/tool/CMakeLists.txt =================================================================== --- clang-tidy/tool/CMakeLists.txt +++ clang-tidy/tool/CMakeLists.txt @@ -13,6 +13,7 @@ clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule + clangTidyModernizeModule clangTidyReadabilityModule clangTooling ) Index: clang-tidy/modernize/PassByValueCheck.h =================================================================== --- clang-tidy/modernize/PassByValueCheck.h +++ clang-tidy/modernize/PassByValueCheck.h @@ -0,0 +1,39 @@ +//===--- PassByValueCheck.h - clang-tidy-------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H + +#include "../ClangTidy.h" +#include "../IncludeInserter.h" + +#include <memory> + +namespace clang { +namespace tidy { +namespace modernize { + +class PassByValueCheck : public ClangTidyCheck { +public: + PassByValueCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerPPCallbacks(clang::CompilerInstance &Compiler) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + std::unique_ptr<IncludeInserter> Inserter; + IncludeSorter::IncludeStyle IncludeStyle; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H Index: clang-tidy/modernize/PassByValueCheck.cpp =================================================================== --- clang-tidy/modernize/PassByValueCheck.cpp +++ clang-tidy/modernize/PassByValueCheck.cpp @@ -0,0 +1,222 @@ +//===--- PassByValueCheck.cpp - clang-tidy---------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "PassByValueCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "clang/Lex/Preprocessor.h" + +using namespace clang::ast_matchers; +using namespace llvm; + +namespace clang { +namespace tidy { +namespace modernize { + +/// \brief Matches move-constructible classes. +/// +/// Given +/// \code +/// // POD types are trivially move constructible. +/// struct Foo { int a; }; +/// +/// struct Bar { +/// Bar(Bar &&) = deleted; +/// int a; +/// }; +/// \endcode +/// recordDecl(isMoveConstructible()) +/// matches "Foo". +AST_MATCHER(CXXRecordDecl, isMoveConstructible) { + for (const CXXConstructorDecl *Ctor : Node.ctors()) { + if (Ctor->isMoveConstructor() && !Ctor->isDeleted()) + return true; + } + return false; +} + +/// \brief Matches non-deleted copy constructors. +/// +/// Given +/// \code +/// struct Foo { Foo(const Foo &) = default; }; +/// struct Bar { Bar(const Bar &) = deleted; }; +/// \endcode +/// constructorDecl(isNonDeletedCopyConstructor()) +/// matches "Foo(const Foo &)". +AST_MATCHER(CXXConstructorDecl, isNonDeletedCopyConstructor) { + return Node.isCopyConstructor() && !Node.isDeleted(); +} + +static TypeMatcher constRefType() { + return lValueReferenceType(pointee(isConstQualified())); +} + +static TypeMatcher nonConstValueType() { + return qualType(unless(anyOf(referenceType(), isConstQualified()))); +} + +/// \brief Whether or not \p ParamDecl is used exactly one time in \p Ctor. +/// +/// Checks both in the init-list and the body of the constructor. +static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor, + const ParmVarDecl *ParamDecl) { + /// \brief \c clang::RecursiveASTVisitor that checks that the given + /// \c ParmVarDecl is used exactly one time. + /// + /// \see ExactlyOneUsageVisitor::hasExactlyOneUsageIn() + class ExactlyOneUsageVisitor + : public RecursiveASTVisitor<ExactlyOneUsageVisitor> { + friend class RecursiveASTVisitor<ExactlyOneUsageVisitor>; + + public: + ExactlyOneUsageVisitor(const ParmVarDecl *ParamDecl) : ParamDecl(ParamDecl) {} + + /// \brief Whether or not the parameter variable is referred only once in the + /// given constructor. + bool hasExactlyOneUsageIn(const CXXConstructorDecl *Ctor) { + Count = 0; + TraverseDecl(const_cast<CXXConstructorDecl *>(Ctor)); + return Count == 1; + } + + private: + /// \brief Counts the number of references to a variable. + /// + /// Stops the AST traversal if more than one usage is found. + bool VisitDeclRefExpr(DeclRefExpr *D) { + if (const ParmVarDecl *To = dyn_cast<ParmVarDecl>(D->getDecl())) { + if (To == ParamDecl) { + ++Count; + if (Count > 1) { + // No need to look further, used more than once. + return false; + } + } + } + return true; + } + + const ParmVarDecl *ParamDecl; + unsigned Count; + }; + + return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor); +} + +/// \brief Find all references to \p ParamDecl across all of the +/// redeclarations of \p Ctor. +static SmallVector<const ParmVarDecl *, 2> collectParamDecls( + const CXXConstructorDecl *Ctor, const ParmVarDecl *ParamDecl) { + SmallVector<const ParmVarDecl *, 2> Results; + unsigned ParamIdx = ParamDecl->getFunctionScopeIndex(); + + for (const FunctionDecl *Redecl : Ctor->redecls()) + Results.push_back(Redecl->getParamDecl(ParamIdx)); + return Results; +} + +PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) { + std::string IncludeStyleStr = Options.get("IncludeStyle", "llvm"); + if (IncludeStyleStr == "llvm") { + IncludeStyle = IncludeSorter::IS_LLVM; + } else { + IncludeStyle = IncludeSorter::IS_Google; + } +} +void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", + IncludeStyle == IncludeSorter::IS_LLVM ? "llvm" : "google"); +} + +void PassByValueCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + constructorDecl( + forEachConstructorInitializer( + ctorInitializer( + // Clang builds a CXXConstructExpr only whin it knows which + // constructor will be called. In dependent contexts a + // ParenListExpr is generated instead of a CXXConstructExpr, + // filtering out templates automatically for us. + withInitializer(constructExpr( + has(declRefExpr(to( + parmVarDecl( + hasType(qualType( + // Match only const-ref or a non-const value + // parameters. Rvalues and const-values + // shouldn't be modified. + anyOf(constRefType(), nonConstValueType())))) + .bind("Param")))), + hasDeclaration(constructorDecl( + isNonDeletedCopyConstructor(), + hasDeclContext(recordDecl(isMoveConstructible()))))))) + .bind("Initializer"))) + .bind("Ctor"), + this); +} + +void PassByValueCheck::registerPPCallbacks(CompilerInstance &Compiler) { + Inserter.reset(new IncludeInserter(Compiler.getSourceManager(), + Compiler.getLangOpts(), + IncludeStyle)); + Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks()); +} + +void PassByValueCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("Ctor"); + const auto *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>("Param"); + const auto *Initializer = + Result.Nodes.getNodeAs<CXXCtorInitializer>("Initializer"); + SourceManager &SM = *Result.SourceManager; + + // If the parameter is used or anything other than the copy, do not apply + // the changes. + if (!paramReferredExactlyOnce(Ctor, ParamDecl)) + return; + + auto Diag = diag(ParamDecl->getLocStart(), "pass by value and use std::move"); + + // Iterate over all declarations of the constructor. + for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) { + auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); + auto RefTL = ParamTL.getAs<ReferenceTypeLoc>(); + + // Do not replace if it is already a value, skip. + if (RefTL.isNull()) + continue; + + TypeLoc ValueTL = RefTL.getPointeeLoc(); + auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getLocStart(), + ParamTL.getLocEnd()); + std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange( + ValueTL.getSourceRange()), SM, Result.Context->getLangOpts()).str(); + ValueStr += ' '; + Diag << FixItHint::CreateReplacement(TypeRange, ValueStr); + } + + // Use std::move in the initialization list. + Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")") + << FixItHint::CreateInsertion( + Initializer->getLParenLoc().getLocWithOffset(1), "std::move("); + + auto Insertion = + Inserter->CreateIncludeInsertion(SM.getMainFileID(), "utility", + /*IsAngled=*/true); + if (Insertion.hasValue()) + Diag << Insertion.getValue(); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang Index: clang-tidy/modernize/ModernizeTidyModule.cpp =================================================================== --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -0,0 +1,46 @@ +//===--- ModernizeTidyModule.cpp - clang-tidy -----------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "../ClangTidy.h" +#include "../ClangTidyModule.h" +#include "../ClangTidyModuleRegistry.h" +#include "PassByValueCheck.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace modernize { + +class ModernizeModule : public ClangTidyModule { +public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value"); + } + + ClangTidyOptions getModuleOptions() override { + ClangTidyOptions Options; + auto &Opts = Options.CheckOptions; + Opts["modernize-pass-by-value.IncludeStyle"] = "llvm"; // Also: "google". + return Options; + } +}; + +// Register the ModernizeTidyModule using this statically initialized variable. +static ClangTidyModuleRegistry::Add<ModernizeModule> X("modernize-module", + "Add modernize checks."); + +} // namespace modernize + +// This anchor is used to force the linker to link in the generated object file +// and thus register the ModernizeModule. +volatile int ModernizeModuleAnchorSource = 0; + +} // namespace tidy +} // namespace clang Index: clang-tidy/modernize/Makefile =================================================================== --- clang-tidy/modernize/Makefile +++ clang-tidy/modernize/Makefile @@ -0,0 +1,12 @@ +##===- clang-tidy/modernize/Makefile -----------------------*- Makefile -*-===## +# +# The LLVM Compiler Infrastructure +# +# This file is distributed under the University of Illinois Open Source +# License. See LICENSE.TXT for details. +# +##===----------------------------------------------------------------------===## +CLANG_LEVEL := ../../../.. +LIBRARYNAME := clangTidyModernizeModule + +include $(CLANG_LEVEL)/Makefile Index: clang-tidy/modernize/CMakeLists.txt =================================================================== --- clang-tidy/modernize/CMakeLists.txt +++ clang-tidy/modernize/CMakeLists.txt @@ -0,0 +1,14 @@ +set(LLVM_LINK_COMPONENTS support) + +add_clang_library(clangTidyModernizeModule + ModernizeTidyModule.cpp + PassByValueCheck.cpp + + LINK_LIBS + clangAST + clangASTMatchers + clangBasic + clangLex + clangTidy + clangTidyReadabilityModule + ) Index: clang-tidy/Makefile =================================================================== --- clang-tidy/Makefile +++ clang-tidy/Makefile @@ -11,6 +11,6 @@ LIBRARYNAME := clangTidy include $(CLANG_LEVEL)/../../Makefile.config -DIRS = utils readability llvm google misc tool +DIRS = utils readability llvm google misc modernize tool include $(CLANG_LEVEL)/Makefile Index: clang-tidy/CMakeLists.txt =================================================================== --- clang-tidy/CMakeLists.txt +++ clang-tidy/CMakeLists.txt @@ -30,5 +30,6 @@ add_subdirectory(llvm) add_subdirectory(google) add_subdirectory(misc) +add_subdirectory(modernize) add_subdirectory(readability) add_subdirectory(utils)
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits