replica
/
hanchenye-llvm-project
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

[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

llvm-svn: 290051
This commit is contained in:
Malcolm Parsons 2016-12-17 20:23:14 +00:00
parent 3aaf11fbd4
commit 79578cc936
10 changed files with 34 additions and 109 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
View File

@ -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

81
clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.cpp
View File

@ -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::registerMatchers(MatchFinder *Finder) {
.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::registerPPCallbacks(CompilerInstance &Compiler) {
void MoveConstructorInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IncludeStyle",
utils::IncludeSorter::toString(IncludeStyle));
Options.store(Opts, "UseCERTSemantics", UseCERTSemantics ? 1 : 0);
}
} // namespace misc

6
clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.h
View File

@ -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

11
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
View File

@ -119,11 +119,13 @@ collectParamDecls(const CXXConstructorDecl *Ctor,
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(MatchFinder *Finder) {
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(MatchFinder *Finder) {
// 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()),

1
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h
View File

@ -30,6 +30,7 @@ public:
private:
std::unique_ptr<utils::IncludeInserter> Inserter;
const utils::IncludeSorter::IncludeStyle IncludeStyle;
const bool ValuesOnly;
};
} // namespace modernize

2
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
View File

@ -95,7 +95,7 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
// 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;

10
clang-tools-extra/docs/ReleaseNotes.rst
View File

@ -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

10
clang-tools-extra/docs/clang-tidy/checks/misc-move-constructor-init.rst
View File

@ -9,9 +9,6 @@ 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
-------
@ -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.

5
clang-tools-extra/docs/clang-tidy/checks/modernize-pass-by-value.rst
View File

@ -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`.

12
clang-tools-extra/test/clang-tidy/misc-move-constructor-init.cpp
View File

@ -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) {}