[clang-tidy] Remove duplicated check from move-constructor-init

[malcolm.parsons]

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.

[flx]

Is the modernize-pass-by-value check configurable in a way to only trigger when copied constructor arguments are not moved?

I think our use case has been to have move-constructor-init check enabled to catch cases that can be optimized but not trigger on every constructor that uses const& instead of value + move, i.e. modernizing is not prescribed, but catching cases where the argument is already passed by value and making them faster is desirable.

[malcolm.parsons]

In D26453#590636, @flx wrote:

Is the modernize-pass-by-value check configurable in a way to only trigger when copied constructor arguments are not moved?

No; good idea.

[malcolm.parsons]

Update comment in performance-unnecessary-value-param check.

[malcolm.parsons]

Add ValuesOnly option to modernize-pass-by-value.

[malcolm.parsons]

Store new option.

[flx]

In D26453#590720, @malcolm.parsons wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

[aaron.ballman]

I think this meets my needs for the cert-oop11-cpp check as it appears to be preserving the original behavior. If a user has the UseCERTSemantics option set in one of their scripts, that appears to be silently accepted and discarded currently, so I don't think this will break anyone's build scripts.

The downside has more to do with the categorization. Not everyone enables "modernize", which is now the only place to be told about a possible correctness issue that misc-move-constructor-init was previously reporting. (This part of the check is one half modernization, one half performance, and one half correctness, depending on which lens you view the code through.) I don't think that's a huge downside, however.

This change should definitely be called out in the release notes.

[malcolm.parsons]

In D26453#596254, @aaron.ballman wrote:

(This part of the check is one half modernization, one half performance, and one half correctness, depending on which lens you view the code through.)

The performance part should be handled by performance-unnecessary-value-param.
It should warn in this situation, but not create a fix that would conflict with modernize-pass-by-value's fix.

[malcolm.parsons]

Mention in release notes.

[malcolm.parsons]

In D26453#592934, @flx wrote:

In D26453#590720, @malcolm.parsons wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

ping.

[malcolm.parsons]

In D26453#610668, @malcolm.parsons wrote:

In D26453#592934, @flx wrote:

In D26453#590720, @malcolm.parsons wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

ping.

@alexfh ping.

[aaron.ballman]

LGTM!

[alexfh]

LG.

In D26453#592934, @flx wrote:

In D26453#590720, @malcolm.parsons wrote:

Add ValuesOnly option to modernize-pass-by-value.

Thanks for doing this. Alex, would this work for us?

Yep, I think so.

clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp

1

Test for one check running a different check is somewhat confusing. If we need to ensure that the patterns the two checks target don't overlap, maybe we should rename the test (<check1>+<check2>.cpp or something like that)?

[malcolm.parsons]

D28022 changes performance-unnecessary-value-param so that it handles this part of modernize-pass-by-value.
So this isn't the end of the story here yet.

clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp

1

Doing it like this was to show that no functionality was lost, not that there's no overlap.
The pass-by-value tests can be moved somewhere else.

clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp

void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FloatLoopCounter>("cert-flp30-c");
     // FIO
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>("cert-fio38-c");
     // ERR
     CheckFactories.registerCheck<StrToNumCheck>("cert-err34-c");
     // MSC
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
   }
-  ClangTidyOptions getModuleOptions() override {
-    ClangTidyOptions Options;
-    Options.CheckOptions["cert-oop11-cpp.UseCERTSemantics"] = "1";
-    return Options;
-  }
 };
 
 } // namespace cert
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<cert::CERTModule>
     X("cert-module",
       "Adds lint checks corresponding to CERT secure coding guidelines.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CERTModule.
 volatile int CERTModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.h

 public:
   MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   void registerPPCallbacks(clang::CompilerInstance &Compiler) override;
   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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H

clang-tools-extra/trunk/clang-tidy/misc/MoveConstructorInitCheck.cpp

 #include "clang/Lex/Preprocessor.h"
 
 using namespace clang::ast_matchers;
 
 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
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus11)
     return;
 
   Finder->addMatcher(
       cxxConstructorDecl(
           unless(isImplicit()),
           allOf(isMoveConstructor(),
                 hasAnyConstructorInitializer(
                     cxxCtorInitializer(
                         withInitializer(cxxConstructExpr(hasDeclaration(
                             cxxConstructorDecl(isCopyConstructor())
                                 .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");
 
   // Do not diagnose if the expression used to perform the initialization is a
   // trivially-copyable type.
   QualType QT = Initializer->getInit()->getType();
   if (QT.isTriviallyCopyableType(*Result.Context))
void MoveConstructorInitCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Inserter.reset(new utils::IncludeInserter(
       Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
   Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
 }
 
 void MoveConstructorInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle",
                 utils::IncludeSorter::toString(IncludeStyle));
-  Options.store(Opts, "UseCERTSemantics", UseCERTSemantics ? 1 : 0);
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.h

public:
   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<utils::IncludeInserter> Inserter;
   const utils::IncludeSorter::IncludeStyle IncludeStyle;
+  const bool ValuesOnly;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H

clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp

collectParamDecls(const CXXConstructorDecl *Ctor,
   for (const FunctionDecl *Redecl : Ctor->redecls())
     Results.push_back(Redecl->getParamDecl(ParamIdx));
   return Results;
 }
 
 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) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus)
     return;
 
   Finder->addMatcher(
       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.
                   withInitializer(cxxConstructExpr(
                       has(ignoringParenImpCasts(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()))))
+                                  ValuesOnly ? nonConstValueType()
+                                             : anyOf(constRefType(),
+                                                     nonConstValueType()))))
                               .bind("Param"))))),
                       hasDeclaration(cxxConstructorDecl(
                           isCopyConstructor(), unless(isDeleted()),
                           hasDeclContext(
                               cxxRecordDecl(isMoveConstructible())))))))
                   .bind("Initializer")))
           .bind("Ctor"),
       this);

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

bool IsConstQualified =
       Param->getType().getCanonicalType().isConstQualified();
 
   // Skip declarations delayed by late template parsing without a body.
   if (!Function->getBody())
     return;
 
   // 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;
 
   auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
       *Param, *Function->getBody(), *Result.Context);
   auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
       *Param, *Function->getBody(), *Result.Context);

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `cppcoreguidelines-slicing
 
   Flags slicing of member variables or vtable.
 
 - New `cppcoreguidelines-special-member-functions
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
 
   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
 
   Warns when ``std::move`` is applied to a forwarding reference instead of
   ``std::forward``.
 
 - `misc-pointer-and-integral-operation` check was removed.
 
 - New `misc-use-after-move
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-use-after-move.html>`_ check
 
   Warns if an object is used after it has been moved, without an intervening
   reinitialization.
 
 - New `cppcoreguidelines-no-malloc
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check
   warns if C-style memory management is used and suggests the use of RAII.
 
 - `modernize-make-unique
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html>`_
   and `modernize-make-shared
   <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
   calling a templated function that behaves as a cast.
 
 - The modernize-use-default check has been renamed to `modernize-use-equals-default
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html>`_.
 

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-constructor-init.rst

 .. title:: clang-tidy - misc-move-constructor-init
 
 misc-move-constructor-init
 ==========================
 
 "cert-oop11-cpp" redirects here as an alias for this check.
 
 The check flags user-defined move constructors that have a ctor-initializer
 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
 -------
 
 .. option:: IncludeStyle
 
    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.

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-pass-by-value.rst

 
 Options
 -------
 
 .. option:: IncludeStyle
 
    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`.

clang-tools-extra/trunk/test/clang-tidy/misc-move-constructor-init.cpp

-// 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>
 
 // CHECK-FIXES: #include <utility>
 
 template <class T> struct remove_reference      {typedef T type;};
 template <class T> struct remove_reference<T&>  {typedef T type;};
 template <class T> struct remove_reference<T&&> {typedef T type;};
struct B {
   B(const B&) {}
   B(B &&) {}
 };
 
 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) {}
 };
 
 struct E : B {
   E() : B() {}
   E(const E &RHS) : B(RHS) {}
   E(E &&RHS) : B(move(RHS)) {} // ok
 };

 struct TriviallyCopyable {
   TriviallyCopyable() = default;
   TriviallyCopyable(TriviallyCopyable &&) = default;
   TriviallyCopyable(const TriviallyCopyable &) = default;
 };
 
 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_;
 };
 
 struct NegativeMultipleInitializerReferences {
   NegativeMultipleInitializerReferences(Movable M) : M_(M), n_(M) {}
   Movable M_;
   Movable n_;
 };
 
 struct NegativeReferencedInConstructorBody {
   NegativeReferencedInConstructorBody(Movable M) : M_(M) { M_ = M; }
   Movable M_;
 };
 
 struct NegativeParamTriviallyCopyable {
   NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
   NegativeParamTriviallyCopyable(int I) : I_(I) {}
 
   TriviallyCopyable T_;
   int I_;
 };
 
 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) {}
   NegativeNotPassedByValue(Movable *M) : M_(*M) {}
   NegativeNotPassedByValue(const Movable *M) : M_(*M) {}
   Movable M_;
 };