flx created this revision.
flx added reviewers: hokein, ymandel, aaron.ballman.
Herald added a subscriber: xazax.hun.
flx requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
This fixes false positive cases where a reference is initialized outside of a
block statement and then its initializing variable is modified. Another case is
when the looped over container is modified.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D103021
Files:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.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
@@ -1,9 +1,19 @@
// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
+template <typename T>
+struct Iterator {
+ void operator++();
+ const T &operator*() const;
+ bool operator!=(const Iterator &) const;
+ typedef const T &const_reference;
+};
+
struct ExpensiveToCopyType {
ExpensiveToCopyType();
virtual ~ExpensiveToCopyType();
const ExpensiveToCopyType &reference() const;
+ Iterator<ExpensiveToCopyType> begin() const;
+ Iterator<ExpensiveToCopyType> end() const;
void nonConstMethod();
bool constMethod() const;
};
@@ -508,3 +518,41 @@
// CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference();
Orig.constMethod();
}
+
+void negativeloopedOverObjectIsModified() {
+ ExpensiveToCopyType Orig;
+ for (const auto &Element : Orig) {
+ const auto Copy = Element;
+ Orig.nonConstMethod();
+ Copy.constMethod();
+ }
+
+ auto Labmda = []() {
+ ExpensiveToCopyType Orig;
+ for (const auto &Element : Orig) {
+ const auto Copy = Element;
+ Orig.nonConstMethod();
+ Copy.constMethod();
+ }
+ };
+}
+
+void negativeReferenceIsInitializedOutsideOfBlock() {
+ ExpensiveToCopyType Orig;
+ const auto &E2 = Orig;
+ if (1 != 2 * 3) {
+ const auto C2 = E2;
+ Orig.nonConstMethod();
+ C2.constMethod();
+ }
+
+ auto Lambda = []() {
+ ExpensiveToCopyType Orig;
+ const auto &E2 = Orig;
+ if (1 != 2 * 3) {
+ const auto C2 = E2;
+ Orig.nonConstMethod();
+ C2.constMethod();
+ }
+ };
+}
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -35,11 +35,12 @@
private:
void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
- bool IssueFix, const VarDecl *ObjectArg,
+ const Stmt &Body, bool IssueFix,
+ const VarDecl *ObjectArg,
ASTContext &Context);
void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
- const Stmt &BlockStmt, bool IssueFix,
- ASTContext &Context);
+ const Stmt &BlockStmt, const Stmt &Body,
+ bool IssueFix, ASTContext &Context);
const std::vector<std::string> AllowedTypes;
};
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
@@ -82,6 +82,7 @@
// object arg or variable that is referenced is immutable as well.
static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
const Stmt &BlockStmt,
+ const Stmt &Body,
ASTContext &Context) {
if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
return false;
@@ -92,12 +93,14 @@
if (!isa<ReferenceType, PointerType>(T))
return true;
+ // Search the whole function body for decl statements of the initialization
+ // variable not just the current block statement.
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.
+ Body, Context);
+ // The reference or pointer is not initialized anywhere witin the function. We
+ // assume its pointee is not modified then.
if (Matches.empty())
return true;
@@ -110,10 +113,10 @@
return true;
// Check that the object argument is immutable as well.
if (const auto *OrigVar = selectFirst<VarDecl>(ObjectArgId, Matches))
- return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+ return isInitializingVariableImmutable(*OrigVar, BlockStmt, Body, 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 isInitializingVariableImmutable(*OrigVar, BlockStmt, Body, Context);
return false;
}
@@ -131,6 +134,7 @@
return compoundStmt(
forEachDescendant(
declStmt(
+ hasAncestor(functionDecl().bind("containingFunc")),
has(varDecl(hasLocalStorage(),
hasType(qualType(
hasCanonicalType(allOf(
@@ -169,6 +173,8 @@
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 *Body =
+ Result.Nodes.getNodeAs<FunctionDecl>("containingFunc")->getBody();
TraversalKindScope RAII(*Result.Context, TK_AsIs);
@@ -186,22 +192,22 @@
return;
if (OldVar == nullptr) {
- handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
+ handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Body, IssueFix, ObjectArg,
*Result.Context);
} else {
- handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
+ handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Body, IssueFix,
*Result.Context);
}
}
void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
- const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
+ const VarDecl &Var, const Stmt &BlockStmt, const Stmt &Body, bool IssueFix,
const VarDecl *ObjectArg, ASTContext &Context) {
bool IsConstQualified = Var.getType().isConstQualified();
if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
return;
if (ObjectArg != nullptr &&
- !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context))
+ !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Body, Context))
return;
auto Diagnostic =
@@ -216,9 +222,9 @@
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
- bool IssueFix, ASTContext &Context) {
+ const Stmt &Body, bool IssueFix, ASTContext &Context) {
if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
- !isInitializingVariableImmutable(OldVar, BlockStmt, Context))
+ !isInitializingVariableImmutable(OldVar, BlockStmt, Body, 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
