flx updated this revision to Diff 351425. flx marked an inline comment as not done. flx added a comment.
Use more efficient method to check for local variable declaration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103021/new/ 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 Lambda = []() { + 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 @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H #include "../ClangTidyCheck.h" +#include "clang/AST/Decl.h" namespace clang { namespace tidy { @@ -35,11 +36,12 @@ private: void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt, - bool IssueFix, const VarDecl *ObjectArg, + const FunctionDecl &Func, 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 FunctionDecl &Func, + 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 @@ -12,6 +12,7 @@ #include "../utils/FixItHintUtils.h" #include "../utils/Matchers.h" #include "../utils/OptionsUtils.h" +#include "clang/AST/Decl.h" #include "clang/Basic/Diagnostic.h" namespace clang { @@ -68,6 +69,16 @@ hasOperatorName("&"), hasUnaryOperand(OldVarDeclRef)))))))); } +bool isDeclaredInFunction(const VarDecl &Var, const DeclContext &Func) { + const DeclContext *DC = Var.getDeclContext(); + while (DC != nullptr) { + if (DC == &Func) + return true; + DC = DC->getLexicalParent(); + } + return false; +} + // 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: @@ -82,6 +93,7 @@ // object arg or variable that is referenced is immutable as well. static bool isInitializingVariableImmutable(const VarDecl &InitializingVar, const Stmt &BlockStmt, + const FunctionDecl &Func, ASTContext &Context) { if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context)) return false; @@ -92,12 +104,20 @@ if (!isa<ReferenceType, PointerType>(T)) return true; + // The reference or pointer is not declared and hence not initialized anywhere + // in the function. We assume its pointee is not modified then. + if (!isDeclaredInFunction(InitializingVar, Func)) { + return true; + } + + // Search the whole function body, not just the current block statement, for + // the decl statement of the initialization variable. 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. + *Func.getBody(), 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 +130,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, Func, 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, Func, Context); return false; } @@ -131,6 +151,7 @@ return compoundStmt( forEachDescendant( declStmt( + hasAncestor(functionDecl().bind("containingFunc")), has(varDecl(hasLocalStorage(), hasType(qualType( hasCanonicalType(allOf( @@ -169,6 +190,7 @@ 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 *Func = Result.Nodes.getNodeAs<FunctionDecl>("containingFunc"); TraversalKindScope RAII(*Result.Context, TK_AsIs); @@ -186,22 +208,22 @@ return; if (OldVar == nullptr) { - handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg, + handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Func, IssueFix, ObjectArg, *Result.Context); } else { - handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix, + handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Func, IssueFix, *Result.Context); } } void UnnecessaryCopyInitialization::handleCopyFromMethodReturn( - const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix, - const VarDecl *ObjectArg, ASTContext &Context) { + const VarDecl &Var, const Stmt &BlockStmt, const FunctionDecl &Func, + 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, Func, Context)) return; auto Diagnostic = @@ -216,9 +238,9 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar( const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, - bool IssueFix, ASTContext &Context) { + const FunctionDecl &Func, bool IssueFix, ASTContext &Context) { if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) || - !isInitializingVariableImmutable(OldVar, BlockStmt, Context)) + !isInitializingVariableImmutable(OldVar, BlockStmt, Func, 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