flx updated this revision to Diff 310888.
flx added a comment.
Added positive variants of new tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91893/new/
https://reviews.llvm.org/D91893
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -476,3 +476,35 @@
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();
+}
Index: clang-tools-extra/clang-tidy/utils/Matchers.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/Matchers.h
+++ clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -43,6 +43,12 @@
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) {
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -58,7 +58,7 @@
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 @@
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;
}
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -19,6 +19,14 @@
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 @@
}
}
-} // 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>(T) && !isa<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 @@
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 @@
.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 @@
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 @@
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 @@
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(),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
