malcolm.parsons created this revision.
malcolm.parsons added reviewers: aaron.ballman, alexfh, flx.
malcolm.parsons added a subscriber: cfe-commits.
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.
https://reviews.llvm.org/D26453
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/misc/MoveConstructorInitCheck.cpp
clang-tidy/misc/MoveConstructorInitCheck.h
clang-tidy/modernize/PassByValueCheck.cpp
docs/clang-tidy/checks/misc-move-constructor-init.rst
test/clang-tidy/misc-move-constructor-init.cpp
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,4 @@
-// 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 -- -- -std=c++11 -isystem %S/Inputs/Headers
#include <s.h>
@@ -96,7 +96,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,7 +121,6 @@
};
struct NegativeNotPassedByValue {
- NegativeNotPassedByValue(const Movable &M) : M_(M) {}
NegativeNotPassedByValue(const Movable M) : M_(M) {}
NegativeNotPassedByValue(Movable &M) : M_(M) {}
NegativeNotPassedByValue(Movable *M) : M_(*M) {}
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,20 +9,10 @@
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.
Index: clang-tidy/modernize/PassByValueCheck.cpp
===================================================================
--- clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tidy/modernize/PassByValueCheck.cpp
@@ -136,7 +136,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.
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<utils::IncludeInserter> 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<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::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IncludeStyle",
utils::IncludeSorter::toString(IncludeStyle));
- Options.store(Opts, "UseCERTSemantics", UseCERTSemantics ? 1 : 0);
}
} // namespace misc
Index: clang-tidy/cert/CERTTidyModule.cpp
===================================================================
--- clang-tidy/cert/CERTTidyModule.cpp
+++ clang-tidy/cert/CERTTidyModule.cpp
@@ -67,11 +67,6 @@
// MSC
CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
}
- ClangTidyOptions getModuleOptions() override {
- ClangTidyOptions Options;
- Options.CheckOptions["cert-oop11-cpp.UseCERTSemantics"] = "1";
- return Options;
- }
};
} // namespace cert
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
