[clang-tools-extra] r290051 - [clang-tidy] Remove duplicated check from move-constructor-init
Malcolm Parsons via cfe-commits
cfe-commits at lists.llvm.org
Sat Dec 17 12:23:15 PST 2016
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) {}
More information about the cfe-commits
mailing list