[clang-tools-extra] 671ad58 - [clang-tidy] performance-unnecessary-copy-initialization: Prevent false positives when dependent variable is modified.
Felix Berger via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 10 13:58:45 PST 2020
Author: Felix Berger
Date: 2020-12-10T16:58:17-05:00
New Revision: 671ad580610ad91139358b7786e02ff70433a90e
URL: https://github.com/llvm/llvm-project/commit/671ad580610ad91139358b7786e02ff70433a90e
DIFF: https://github.com/llvm/llvm-project/commit/671ad580610ad91139358b7786e02ff70433a90e.diff
LOG: [clang-tidy] performance-unnecessary-copy-initialization: Prevent false positives when dependent variable is modified.
Extend the check to not only look at the variable the unnecessarily copied
variable is initialized from, but also ensure that any variable the old variable
references is not modified.
Extend DeclRefExprUtils to also count references and pointers to const assigned
from the DeclRef we check for const usages.
Reviewed-by: aaron.ballman
Differential Revision: https://reviews.llvm.org/D91893
Added:
Modified:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
clang-tools-extra/clang-tidy/utils/Matchers.h
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 24847d80657c..f6b9365435fb 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -19,6 +19,14 @@ namespace tidy {
namespace performance {
namespace {
+using namespace ::clang::ast_matchers;
+using llvm::StringRef;
+using utils::decl_ref_expr::isOnlyUsedAsConst;
+
+static constexpr StringRef ObjectArgId = "objectArg";
+static constexpr StringRef InitFunctionCallId = "initFunctionCall";
+static constexpr StringRef OldVarDeclId = "oldVarDecl";
+
void recordFixes(const VarDecl &Var, ASTContext &Context,
DiagnosticBuilder &Diagnostic) {
Diagnostic << utils::fixit::changeVarDeclToReference(Var, Context);
@@ -29,10 +37,88 @@ void recordFixes(const VarDecl &Var, ASTContext &Context,
}
}
-} // namespace
+AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) {
+ // Match method call expressions where the `this` argument is only used as
+ // const, this will be checked in `check()` part. This returned const
+ // reference is highly likely to outlive the local const reference of the
+ // variable being declared. The assumption is that the const reference being
+ // returned either points to a global static variable or to a member of the
+ // called object.
+ return cxxMemberCallExpr(
+ callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))),
+ on(declRefExpr(to(varDecl().bind(ObjectArgId)))));
+}
-using namespace ::clang::ast_matchers;
-using utils::decl_ref_expr::isOnlyUsedAsConst;
+AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
+ // Only allow initialization of a const reference from a free function if it
+ // has no arguments. Otherwise it could return an alias to one of its
+ // arguments and the arguments need to be checked for const use as well.
+ return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst()))),
+ argumentCountIs(0), unless(callee(cxxMethodDecl())))
+ .bind(InitFunctionCallId);
+}
+
+AST_MATCHER_FUNCTION(StatementMatcher, isInitializedFromReferenceToConst) {
+ auto OldVarDeclRef =
+ declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
+ return declStmt(has(varDecl(hasInitializer(
+ anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(),
+ ignoringImpCasts(OldVarDeclRef),
+ ignoringImpCasts(unaryOperator(
+ hasOperatorName("&"), hasUnaryOperand(OldVarDeclRef))))))));
+}
+
+// This checks that the variable itself is only used as const, and also makes
+// sure that it does not reference another variable that could be modified in
+// the BlockStmt. It does this by checking the following:
+// 1. If the variable is neither a reference nor a pointer then the
+// isOnlyUsedAsConst() check is sufficient.
+// 2. If the (reference or pointer) variable is not initialized in a DeclStmt in
+// the BlockStmt. In this case its pointee is likely not modified (unless it
+// is passed as an alias into the method as well).
+// 3. If the reference is initialized from a reference to const. This is
+// the same set of criteria we apply when identifying the unnecessary copied
+// variable in this check to begin with. In this case we check whether the
+// object arg or variable that is referenced is immutable as well.
+static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
+ const Stmt &BlockStmt,
+ ASTContext &Context) {
+ if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+ return false;
+
+ QualType T = InitializingVar.getType();
+ // The variable is a value type and we know it is only used as const. Safe
+ // to reference it and avoid the copy.
+ if (!isa<ReferenceType, PointerType>(T))
+ return true;
+
+ auto Matches =
+ match(findAll(declStmt(has(varDecl(equalsNode(&InitializingVar))))
+ .bind("declStmt")),
+ BlockStmt, Context);
+ // The reference or pointer is not initialized in the BlockStmt. We assume
+ // its pointee is not modified then.
+ if (Matches.empty())
+ return true;
+
+ const auto *Initialization = selectFirst<DeclStmt>("declStmt", Matches);
+ Matches =
+ match(isInitializedFromReferenceToConst(), *Initialization, Context);
+ // The reference is initialized from a free function without arguments
+ // returning a const reference. This is a global immutable object.
+ if (selectFirst<CallExpr>(InitFunctionCallId, Matches) != nullptr)
+ return true;
+ // Check that the object argument is immutable as well.
+ if (const auto *OrigVar = selectFirst<VarDecl>(ObjectArgId, Matches))
+ return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+ // Check that the old variable we reference is immutable as well.
+ if (const auto *OrigVar = selectFirst<VarDecl>(OldVarDeclId, Matches))
+ return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+
+ return false;
+}
+
+} // namespace
UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
StringRef Name, ClangTidyContext *Context)
@@ -41,22 +127,6 @@ UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
- auto ConstReference = referenceType(pointee(qualType(isConstQualified())));
-
- // Match method call expressions where the `this` argument is only used as
- // const, this will be checked in `check()` part. This returned const
- // reference is highly likely to outlive the local const reference of the
- // variable being declared. The assumption is that the const reference being
- // returned either points to a global static variable or to a member of the
- // called object.
- auto ConstRefReturningMethodCall =
- cxxMemberCallExpr(callee(cxxMethodDecl(returns(ConstReference))),
- on(declRefExpr(to(varDecl().bind("objectArg")))));
- auto ConstRefReturningFunctionCall =
- callExpr(callee(functionDecl(returns(ConstReference))),
- unless(callee(cxxMethodDecl())))
- .bind("initFunctionCall");
-
auto localVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
return compoundStmt(
forEachDescendant(
@@ -83,24 +153,22 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
.bind("blockStmt");
};
- Finder->addMatcher(localVarCopiedFrom(anyOf(ConstRefReturningFunctionCall,
- ConstRefReturningMethodCall)),
+ Finder->addMatcher(localVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
+ isConstRefReturningMethodCall())),
this);
Finder->addMatcher(localVarCopiedFrom(declRefExpr(
- to(varDecl(hasLocalStorage()).bind("oldVarDecl")))),
+ to(varDecl(hasLocalStorage()).bind(OldVarDeclId)))),
this);
}
void UnnecessaryCopyInitialization::check(
const MatchFinder::MatchResult &Result) {
const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
- const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>("oldVarDecl");
- const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg");
+ const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>(OldVarDeclId);
+ const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>(ObjectArgId);
const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
- const auto *InitFunctionCall =
- Result.Nodes.getNodeAs<CallExpr>("initFunctionCall");
TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs);
@@ -118,11 +186,6 @@ void UnnecessaryCopyInitialization::check(
return;
if (OldVar == nullptr) {
- // Only allow initialization of a const reference from a free function if it
- // has no arguments. Otherwise it could return an alias to one of its
- // arguments and the arguments need to be checked for const use as well.
- if (InitFunctionCall != nullptr && InitFunctionCall->getNumArgs() > 0)
- return;
handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
*Result.Context);
} else {
@@ -138,7 +201,7 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
return;
if (ObjectArg != nullptr &&
- !isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context))
+ !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context))
return;
auto Diagnostic =
@@ -158,7 +221,7 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
bool IssueFix, ASTContext &Context) {
if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
- !isOnlyUsedAsConst(OldVar, BlockStmt, Context))
+ !isInitializingVariableImmutable(OldVar, BlockStmt, Context))
return;
auto Diagnostic = diag(NewVar.getLocation(),
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index d0933f0860c8..10d2b24a9677 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -58,7 +58,7 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
extractNodesByIdTo(Matches, "declRef", DeclRefs);
auto ConstReferenceOrValue =
- qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))),
+ qualType(anyOf(matchers::isReferenceToConst(),
unless(anyOf(referenceType(), pointerType(),
substTemplateTypeParmType()))));
auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
@@ -71,6 +71,20 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
Matches =
match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ // References and pointers to const assignments.
+ Matches =
+ match(findAll(declStmt(
+ has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
+ hasInitializer(ignoringImpCasts(DeclRefToVar)))))),
+ Stmt, Context);
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ Matches =
+ match(findAll(declStmt(has(varDecl(
+ hasType(qualType(matchers::isPointerToConst())),
+ hasInitializer(ignoringImpCasts(unaryOperator(
+ hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
+ Stmt, Context);
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
return DeclRefs;
}
diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h
index 348ab758f3d4..9b765aedd13a 100644
--- a/clang-tools-extra/clang-tidy/utils/Matchers.h
+++ b/clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -43,6 +43,12 @@ AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
return referenceType(pointee(qualType(isConstQualified())));
}
+// Returns QualType matcher for pointers to const.
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isPointerToConst) {
+ using namespace ast_matchers;
+ return pointerType(pointee(qualType(isConstQualified())));
+}
+
AST_MATCHER_P(NamedDecl, matchesAnyListedName, std::vector<std::string>,
NameList) {
return llvm::any_of(NameList, [&Node](const std::string &Name) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
index 120902d0eade..8cc095e431fd 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -476,3 +476,35 @@ void negativeInvokedOnStdFunction(
auto Copy = Orig.reference();
Update(Copy);
}
+
+void negativeCopiedFromReferenceToModifiedVar() {
+ ExpensiveToCopyType Orig;
+ const auto &Ref = Orig;
+ const auto NecessaryCopy = Ref;
+ Orig.nonConstMethod();
+}
+
+void positiveCopiedFromReferenceToConstVar() {
+ ExpensiveToCopyType Orig;
+ const auto &Ref = Orig;
+ const auto UnnecessaryCopy = Ref;
+ // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'UnnecessaryCopy' of
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Ref;
+ Orig.constMethod();
+}
+
+void negativeCopiedFromGetterOfReferenceToModifiedVar() {
+ ExpensiveToCopyType Orig;
+ const auto &Ref = Orig.reference();
+ const auto NecessaryCopy = Ref.reference();
+ Orig.nonConstMethod();
+}
+
+void positiveCopiedFromGetterOfReferenceToConstVar() {
+ ExpensiveToCopyType Orig;
+ const auto &Ref = Orig.reference();
+ auto UnnecessaryCopy = Ref.reference();
+ // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'UnnecessaryCopy' is
+ // CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference();
+ Orig.constMethod();
+}
More information about the cfe-commits
mailing list