Index: clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tidy/cert/CERTTidyModule.cpp +++ clang-tidy/cert/CERTTidyModule.cpp @@ -67,11 +67,6 @@ // MSC CheckFactories.registerCheck("cert-msc30-c"); } - ClangTidyOptions getModuleOptions() override { - ClangTidyOptions Options; - Options.CheckOptions["cert-oop11-cpp.UseCERTSemantics"] = "1"; - return Options; - } }; } // namespace cert Index: clang-tidy/misc/MoveConstructorInitCheck.h =================================================================== --- clang-tidy/misc/MoveConstructorInitCheck.h +++ clang-tidy/misc/MoveConstructorInitCheck.h @@ -33,14 +33,8 @@ 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 Inserter; const utils::IncludeSorter::IncludeStyle IncludeStyle; - const bool UseCERTSemantics; }; } // namespace misc Index: clang-tidy/misc/MoveConstructorInitCheck.cpp =================================================================== --- clang-tidy/misc/MoveConstructorInitCheck.cpp +++ clang-tidy/misc/MoveConstructorInitCheck.cpp @@ -21,30 +21,11 @@ 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 @@ .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("move-init") != nullptr) - handleMoveConstructor(Result); - if (Result.Nodes.getNodeAs("movable-param") != nullptr) - handleParamNotMoved(Result); -} - -void MoveConstructorInitCheck::handleParamNotMoved( - const MatchFinder::MatchResult &Result) { - const auto *MovableParam = - Result.Nodes.getNodeAs("movable-param"); - const auto *ConstructorDecl = - Result.Nodes.getNodeAs("ctor-decl"); - const auto *InitArg = Result.Nodes.getNodeAs("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("ctor"); const auto *Initializer = Result.Nodes.getNodeAs("move-init"); @@ -178,7 +100,6 @@ void MoveConstructorInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IncludeStyle", utils::IncludeSorter::toString(IncludeStyle)); - Options.store(Opts, "UseCERTSemantics", UseCERTSemantics ? 1 : 0); } } // namespace misc Index: clang-tidy/modernize/PassByValueCheck.h =================================================================== --- clang-tidy/modernize/PassByValueCheck.h +++ clang-tidy/modernize/PassByValueCheck.h @@ -30,6 +30,7 @@ private: std::unique_ptr Inserter; const utils::IncludeSorter::IncludeStyle IncludeStyle; + const bool ValuesOnly; }; } // namespace modernize Index: clang-tidy/modernize/PassByValueCheck.cpp =================================================================== --- clang-tidy/modernize/PassByValueCheck.cpp +++ clang-tidy/modernize/PassByValueCheck.cpp @@ -119,11 +119,13 @@ 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 @@ 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 @@ // 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()), Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp =================================================================== --- clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -83,7 +83,7 @@ // 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(Function) || !Function->doesThisDeclarationHaveABody())) return; Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -67,6 +67,11 @@ Flags classes where some, but not all, special member functions are user-defined. +- The UseCERTSemantics option for the `misc-move-constructor-init + `_ check + has been removed as it duplicated the `modernize-pass-by-value + `_ check. + - New `misc-move-forwarding-reference `_ check @@ -87,6 +92,11 @@ `_ now handle calls to the smart pointer's ``reset()`` method. +- The `modernize-pass-by-value + `_ check + now has a ValuesOnly option to only warn about parameters that are passed by + value but not moved. + - The `modernize-use-auto `_ check now warns about variable declarations that are initialized with a cast. Index: docs/clang-tidy/checks/misc-move-constructor-init.rst =================================================================== --- docs/clang-tidy/checks/misc-move-constructor-init.rst +++ docs/clang-tidy/checks/misc-move-constructor-init.rst @@ -9,9 +9,6 @@ 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 @@ 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 `_. - Default is `0` for misc-move-constructor-init and `1` for cert-oop11-cpp. Index: docs/clang-tidy/checks/modernize-pass-by-value.rst =================================================================== --- docs/clang-tidy/checks/modernize-pass-by-value.rst +++ docs/clang-tidy/checks/modernize-pass-by-value.rst @@ -159,3 +159,8 @@ 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`. Index: test/clang-tidy/misc-move-constructor-init.cpp =================================================================== --- test/clang-tidy/misc-move-constructor-init.cpp +++ test/clang-tidy/misc-move-constructor-init.cpp @@ -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 @@ -28,8 +31,8 @@ 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 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 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) {}