Improved the misc-move-constructor-init check to identify arguments that are passed by value but copy assigned to class data members when the non-deleted move constructor is a better fit.
Patch by Felix Berger!
llvm-svn: 249429
diff --git a/clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.cpp b/clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.cpp
index 0ba0f16..ba182f3 100644
--- a/clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.cpp
@@ -8,14 +8,42 @@
//===----------------------------------------------------------------------===//
#include "MoveConstructorInitCheck.h"
+#include "../utils/Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
using namespace clang::ast_matchers;
namespace clang {
namespace tidy {
+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(IncludeSorter::parseIncludeStyle(
+ 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.
@@ -31,13 +59,65 @@
withInitializer(cxxConstructExpr(hasDeclaration(
cxxConstructorDecl(isCopyConstructor())
.bind("ctor")))))
- .bind("init")))),
+ .bind("move-init")))),
+ this);
+
+ auto NonConstValueMovableAndExpensiveToCopy =
+ qualType(allOf(unless(pointerType()), unless(isConstQualified()),
+ hasDeclaration(cxxRecordDecl(hasMethod(cxxConstructorDecl(
+ isMoveConstructor(), unless(isDeleted()))))),
+ matchers::isExpensiveToCopy()));
+ 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 can be moved to avoid copy");
+ 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>("init");
+ 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.
@@ -79,6 +159,15 @@
}
}
+void MoveConstructorInitCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+ Inserter.reset(new IncludeInserter(Compiler.getSourceManager(),
+ Compiler.getLangOpts(), IncludeStyle));
+ Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void MoveConstructorInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IncludeStyle", IncludeSorter::toString(IncludeStyle));
+}
+
} // namespace tidy
} // namespace clang
-