Author: malcolm.parsons Date: Sat Dec 17 14:23:14 2016 New Revision: 290051
URL: http://llvm.org/viewvc/llvm-project?rev=290051&view=rev Log: [clang-tidy] Remove duplicated check from move-constructor-init Summary: An addition to the move-constructor-init check was duplicating the modernize-pass-by-value check. Remove the additional check and UseCERTSemantics option. Run the move-constructor-init test with both checks enabled. Fix modernize-pass-by-value false-positive when initializing a base class. Add option to modernize-pass-by-value to only warn about parameters that are already values. Reviewers: alexfh, flx, aaron.ballman Subscribers: cfe-commits Differential Revision: https://reviews.llvm.org/D26453 Modified: clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.cpp clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.h clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.h clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp clang-tools-extra/trunk/docs/ReleaseNotes.rst clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-constructor-init.rst clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-pass-by-value.rst clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp Modified: clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp Sat Dec 17 14:23:14 2016 @@ -67,11 +67,6 @@ public: // MSC CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c"); } - ClangTidyOptions getModuleOptions() override { - ClangTidyOptions Options; - Options.CheckOptions["cert-oop11-cpp.UseCERTSemantics"] = "1"; - return Options; - } }; } // namespace cert Modified: clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.cpp?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.cpp Sat Dec 17 14:23:14 2016 @@ -21,30 +21,11 @@ namespace clang { namespace tidy { namespace misc { -namespace { - -unsigned int -parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam, - const CXXConstructorDecl &ConstructorDecl, - ASTContext &Context) { - unsigned int Occurrences = 0; - auto AllDeclRefs = - findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam))))); - Occurrences += match(AllDeclRefs, *ConstructorDecl.getBody(), Context).size(); - for (const auto *Initializer : ConstructorDecl.inits()) { - Occurrences += match(AllDeclRefs, *Initializer->getInit(), Context).size(); - } - return Occurrences; -} - -} // namespace - MoveConstructorInitCheck::MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), IncludeStyle(utils::IncludeSorter::parseIncludeStyle( - Options.get("IncludeStyle", "llvm"))), - UseCERTSemantics(Options.get("UseCERTSemantics", 0) != 0) {} + Options.get("IncludeStyle", "llvm"))) {} void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) { // Only register the matchers for C++11; the functionality currently does not @@ -63,68 +44,9 @@ void MoveConstructorInitCheck::registerM .bind("ctor"))))) .bind("move-init")))), this); - - auto NonConstValueMovableAndExpensiveToCopy = - qualType(allOf(unless(pointerType()), unless(isConstQualified()), - hasDeclaration(cxxRecordDecl(hasMethod(cxxConstructorDecl( - isMoveConstructor(), unless(isDeleted()))))), - matchers::isExpensiveToCopy())); - - // This checker is also used to implement cert-oop11-cpp, but when using that - // form of the checker, we do not want to diagnose movable parameters. - if (!UseCERTSemantics) { - Finder->addMatcher( - cxxConstructorDecl( - allOf( - unless(isMoveConstructor()), - hasAnyConstructorInitializer(withInitializer(cxxConstructExpr( - hasDeclaration(cxxConstructorDecl(isCopyConstructor())), - hasArgument( - 0, - declRefExpr( - to(parmVarDecl( - hasType( - NonConstValueMovableAndExpensiveToCopy)) - .bind("movable-param"))) - .bind("init-arg"))))))) - .bind("ctor-decl"), - this); - } } void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) { - if (Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init") != nullptr) - handleMoveConstructor(Result); - if (Result.Nodes.getNodeAs<ParmVarDecl>("movable-param") != nullptr) - handleParamNotMoved(Result); -} - -void MoveConstructorInitCheck::handleParamNotMoved( - const MatchFinder::MatchResult &Result) { - const auto *MovableParam = - Result.Nodes.getNodeAs<ParmVarDecl>("movable-param"); - const auto *ConstructorDecl = - Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor-decl"); - const auto *InitArg = Result.Nodes.getNodeAs<DeclRefExpr>("init-arg"); - // If the parameter is referenced more than once it is not safe to move it. - if (parmVarDeclRefExprOccurences(*MovableParam, *ConstructorDecl, - *Result.Context) > 1) - return; - auto DiagOut = diag(InitArg->getLocStart(), - "value argument %0 can be moved to avoid copy") - << MovableParam; - DiagOut << FixItHint::CreateReplacement( - InitArg->getSourceRange(), - (Twine("std::move(") + MovableParam->getName() + ")").str()); - if (auto IncludeFixit = Inserter->CreateIncludeInsertion( - Result.SourceManager->getFileID(InitArg->getLocStart()), "utility", - /*IsAngled=*/true)) { - DiagOut << *IncludeFixit; - } -} - -void MoveConstructorInitCheck::handleMoveConstructor( - const MatchFinder::MatchResult &Result) { const auto *CopyCtor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor"); const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init"); @@ -178,7 +100,6 @@ void MoveConstructorInitCheck::registerP void MoveConstructorInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IncludeStyle", utils::IncludeSorter::toString(IncludeStyle)); - Options.store(Opts, "UseCERTSemantics", UseCERTSemantics ? 1 : 0); } } // namespace misc Modified: clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.h?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.h (original) +++ clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.h Sat Dec 17 14:23:14 2016 @@ -33,14 +33,8 @@ public: void storeOptions(ClangTidyOptions::OptionMap &Opts) override; private: - void - handleMoveConstructor(const ast_matchers::MatchFinder::MatchResult &Result); - void - handleParamNotMoved(const ast_matchers::MatchFinder::MatchResult &Result); - std::unique_ptr<utils::IncludeInserter> Inserter; const utils::IncludeSorter::IncludeStyle IncludeStyle; - const bool UseCERTSemantics; }; } // namespace misc Modified: clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp Sat Dec 17 14:23:14 2016 @@ -119,11 +119,13 @@ collectParamDecls(const CXXConstructorDe PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), IncludeStyle(utils::IncludeSorter::parseIncludeStyle( - Options.get("IncludeStyle", "llvm"))) {} + Options.get("IncludeStyle", "llvm"))), + ValuesOnly(Options.get("ValuesOnly", 0) != 0) {} void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IncludeStyle", utils::IncludeSorter::toString(IncludeStyle)); + Options.store(Opts, "ValuesOnly", ValuesOnly); } void PassByValueCheck::registerMatchers(MatchFinder *Finder) { @@ -136,7 +138,8 @@ void PassByValueCheck::registerMatchers( cxxConstructorDecl( forEachConstructorInitializer( cxxCtorInitializer( - // Clang builds a CXXConstructExpr only whin it knows which + unless(isBaseInitializer()), + // Clang builds a CXXConstructExpr only when it knows which // constructor will be called. In dependent contexts a // ParenListExpr is generated instead of a CXXConstructExpr, // filtering out templates automatically for us. @@ -147,7 +150,9 @@ void PassByValueCheck::registerMatchers( // Match only const-ref or a non-const value // parameters. Rvalues and const-values // shouldn't be modified. - anyOf(constRefType(), nonConstValueType())))) + ValuesOnly ? nonConstValueType() + : anyOf(constRefType(), + nonConstValueType())))) .bind("Param"))))), hasDeclaration(cxxConstructorDecl( isCopyConstructor(), unless(isDeleted()), Modified: clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.h?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.h (original) +++ clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.h Sat Dec 17 14:23:14 2016 @@ -30,6 +30,7 @@ public: private: std::unique_ptr<utils::IncludeInserter> Inserter; const utils::IncludeSorter::IncludeStyle IncludeStyle; + const bool ValuesOnly; }; } // namespace modernize Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp Sat Dec 17 14:23:14 2016 @@ -95,7 +95,7 @@ void UnnecessaryValueParamCheck::check(c // Do not trigger on non-const value parameters when: // 1. they are in a constructor definition since they can likely trigger - // misc-move-constructor-init which will suggest to move the argument. + // modernize-pass-by-value which will suggest to move the argument. if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) || !Function->doesThisDeclarationHaveABody())) return; Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original) +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Sat Dec 17 14:23:14 2016 @@ -67,6 +67,11 @@ Improvements to clang-tidy Flags classes where some, but not all, special member functions are user-defined. +- The UseCERTSemantics option for the `misc-move-constructor-init + <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-constructor-init.html>`_ check + has been removed as it duplicated the `modernize-pass-by-value + <http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html>`_ check. + - New `misc-move-forwarding-reference <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check @@ -91,6 +96,11 @@ Improvements to clang-tidy <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_ now handle calls to the smart pointer's ``reset()`` method. +- The `modernize-pass-by-value + <http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html>`_ check + now has a ValuesOnly option to only warn about parameters that are passed by + value but not moved. + - The `modernize-use-auto <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>`_ check now warns about variable declarations that are initialized with a cast, or by Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-constructor-init.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-constructor-init.rst?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-constructor-init.rst (original) +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-constructor-init.rst Sat Dec 17 14:23:14 2016 @@ -9,9 +9,6 @@ The check flags user-defined move constr initializing a member or base class through a copy constructor instead of a move constructor. -It also flags constructor arguments that are passed by value, have a non-deleted -move-constructor and are assigned to a class field by copy construction. - Options ------- @@ -19,10 +16,3 @@ Options A string specifying which include-style is used, `llvm` or `google`. Default is `llvm`. - -.. option:: UseCERTSemantics - - When non-zero, the check conforms to the behavior expected by the CERT secure - coding recommendation - `OOP11-CPP <https://www.securecoding.cert.org/confluence/display/cplusplus/OOP11-CPP.+Do+not+copy-initialize+members+or+base+classes+from+a+move+constructor>`_. - Default is `0` for misc-move-constructor-init and `1` for cert-oop11-cpp. Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-pass-by-value.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-pass-by-value.rst?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-pass-by-value.rst (original) +++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-pass-by-value.rst Sat Dec 17 14:23:14 2016 @@ -159,3 +159,8 @@ Options A string specifying which include-style is used, `llvm` or `google`. Default is `llvm`. + +.. option:: ValuesOnly + + When non-zero, the check only warns about copied parameters that are already + passed by value. Default is `0`. Modified: clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp?rev=290051&r1=290050&r2=290051&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp Sat Dec 17 14:23:14 2016 @@ -1,4 +1,7 @@ -// RUN: %check_clang_tidy %s misc-move-constructor-init %t -- -- -std=c++11 -isystem %S/Inputs/Headers +// RUN: %check_clang_tidy %s misc-move-constructor-init,modernize-pass-by-value %t -- \ +// RUN: -config='{CheckOptions: \ +// RUN: [{key: modernize-pass-by-value.ValuesOnly, value: 1}]}' \ +// RUN: -- -std=c++11 -isystem %S/Inputs/Headers #include <s.h> @@ -28,8 +31,8 @@ struct D : B { D() : B() {} D(const D &RHS) : B(RHS) {} // CHECK-MESSAGES: :[[@LINE+3]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-init] - // CHECK-MESSAGES: 23:3: note: copy constructor being called - // CHECK-MESSAGES: 24:3: note: candidate move constructor here + // CHECK-MESSAGES: 26:3: note: copy constructor being called + // CHECK-MESSAGES: 27:3: note: candidate move constructor here D(D &&RHS) : B(RHS) {} }; @@ -96,7 +99,7 @@ struct TriviallyCopyable { struct Positive { Positive(Movable M) : M_(M) {} - // CHECK-MESSAGES: [[@LINE-1]]:28: warning: value argument 'M' can be moved to avoid copy [misc-move-constructor-init] + // CHECK-MESSAGES: [[@LINE-1]]:12: warning: pass by value and use std::move [modernize-pass-by-value] // CHECK-FIXES: Positive(Movable M) : M_(std::move(M)) {} Movable M_; }; @@ -121,6 +124,7 @@ struct NegativeParamTriviallyCopyable { }; struct NegativeNotPassedByValue { + // This const ref constructor isn't warned about because the ValuesOnly option is set. NegativeNotPassedByValue(const Movable &M) : M_(M) {} NegativeNotPassedByValue(const Movable M) : M_(M) {} NegativeNotPassedByValue(Movable &M) : M_(M) {} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits