From 5dbe3bf4b8dbb7e67d41c7c1360f15d512dd72a0 Mon Sep 17 00:00:00 2001 From: Felix Berger Date: Mon, 10 May 2021 11:26:00 -0400 Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-initialization: Remove the complete statement when the copied variable is unused. It is not useful to keep the statement around and can lead to compiler warnings when -Wall (-Wunused-variable specifically) turned on. Differential Revision: https://reviews.llvm.org/D102175 Reviewed-by: ymandel --- .../UnnecessaryCopyInitialization.cpp | 87 ++++++++++++++----- .../UnnecessaryCopyInitialization.h | 7 +- ...sary-copy-initialization-allowed-types.cpp | 2 + ...rmance-unnecessary-copy-initialization.cpp | 61 +++++++++++++ 4 files changed, 130 insertions(+), 27 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp index 0cc9a4426302..57a2310e779f 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -7,9 +7,9 @@ //===----------------------------------------------------------------------===// #include "UnnecessaryCopyInitialization.h" - #include "../utils/DeclRefExprUtils.h" #include "../utils/FixItHintUtils.h" +#include "../utils/LexerUtils.h" #include "../utils/Matchers.h" #include "../utils/OptionsUtils.h" #include "clang/Basic/Diagnostic.h" @@ -21,6 +21,7 @@ namespace { using namespace ::clang::ast_matchers; using llvm::StringRef; +using utils::decl_ref_expr::allDeclRefExprs; using utils::decl_ref_expr::isOnlyUsedAsConst; static constexpr StringRef ObjectArgId = "objectArg"; @@ -37,6 +38,19 @@ void recordFixes(const VarDecl &Var, ASTContext &Context, } } +void recordRemoval(const DeclStmt &Stmt, ASTContext &Context, + DiagnosticBuilder &Diagnostic) { + // Attempt to remove the whole line until the next non-comment token. + auto Tok = utils::lexer::findNextTokenSkippingComments( + Stmt.getEndLoc(), Context.getSourceManager(), Context.getLangOpts()); + if (Tok) { + Diagnostic << FixItHint::CreateRemoval(SourceRange( + Stmt.getBeginLoc(), Tok->getLocation().getLocWithOffset(-1))); + } else { + Diagnostic << FixItHint::CreateRemoval(Stmt.getSourceRange()); + } +} + 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 @@ -118,6 +132,11 @@ static bool isInitializingVariableImmutable(const VarDecl &InitializingVar, return false; } +bool isVariableUnused(const VarDecl &Var, const Stmt &BlockStmt, + ASTContext &Context) { + return allDeclRefExprs(Var, BlockStmt, Context).empty(); +} + } // namespace UnnecessaryCopyInitialization::UnnecessaryCopyInitialization( @@ -169,14 +188,13 @@ void UnnecessaryCopyInitialization::check( const auto *ObjectArg = Result.Nodes.getNodeAs(ObjectArgId); const auto *BlockStmt = Result.Nodes.getNodeAs("blockStmt"); const auto *CtorCall = Result.Nodes.getNodeAs("ctorCall"); + const auto *Stmt = Result.Nodes.getNodeAs("declStmt"); TraversalKindScope RAII(*Result.Context, TK_AsIs); // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros // since we cannot place them correctly. - bool IssueFix = - Result.Nodes.getNodeAs("declStmt")->isSingleDecl() && - !NewVar->getLocation().isMacroID(); + bool IssueFix = Stmt->isSingleDecl() && !NewVar->getLocation().isMacroID(); // A constructor that looks like T(const T& t, bool arg = false) counts as a // copy only when it is called with default arguments for the arguments after @@ -186,47 +204,68 @@ void UnnecessaryCopyInitialization::check( return; if (OldVar == nullptr) { - handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg, + handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg, *Result.Context); } else { - handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix, + handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, 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 DeclStmt &Stmt, + 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)) return; - - auto Diagnostic = - diag(Var.getLocation(), - "the %select{|const qualified }0variable %1 is copy-constructed " - "from a const reference%select{ but is only used as const " - "reference|}0; consider making it a const reference") - << IsConstQualified << &Var; - if (IssueFix) - recordFixes(Var, Context, Diagnostic); + if (isVariableUnused(Var, BlockStmt, Context)) { + auto Diagnostic = + diag(Var.getLocation(), + "the %select{|const qualified }0variable %1 is copy-constructed " + "from a const reference but is never used; consider " + "removing the statement") + << IsConstQualified << &Var; + if (IssueFix) + recordRemoval(Stmt, Context, Diagnostic); + } else { + auto Diagnostic = + diag(Var.getLocation(), + "the %select{|const qualified }0variable %1 is copy-constructed " + "from a const reference%select{ but is only used as const " + "reference|}0; consider making it a const reference") + << IsConstQualified << &Var; + if (IssueFix) + recordFixes(Var, Context, Diagnostic); + } } void UnnecessaryCopyInitialization::handleCopyFromLocalVar( const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, - bool IssueFix, ASTContext &Context) { + const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) { if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) || !isInitializingVariableImmutable(OldVar, BlockStmt, Context)) return; - auto Diagnostic = diag(NewVar.getLocation(), - "local copy %0 of the variable %1 is never modified; " - "consider avoiding the copy") - << &NewVar << &OldVar; - if (IssueFix) - recordFixes(NewVar, Context, Diagnostic); + if (isVariableUnused(NewVar, BlockStmt, Context)) { + auto Diagnostic = diag(NewVar.getLocation(), + "local copy %0 of the variable %1 is never modified " + "and never used; " + "consider removing the statement") + << &NewVar << &OldVar; + if (IssueFix) + recordRemoval(Stmt, Context, Diagnostic); + } else { + auto Diagnostic = + diag(NewVar.getLocation(), + "local copy %0 of the variable %1 is never modified; " + "consider avoiding the copy") + << &NewVar << &OldVar; + if (IssueFix) + recordFixes(NewVar, Context, Diagnostic); + } } void UnnecessaryCopyInitialization::storeOptions( diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h index eeb9138d8532..819367221893 100644 --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h +++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -35,11 +35,12 @@ public: private: void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt, - bool IssueFix, const VarDecl *ObjectArg, + const DeclStmt &Stmt, 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 DeclStmt &Stmt, + bool IssueFix, ASTContext &Context); const std::vector AllowedTypes; }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp index 420efa8dbe3c..52b14be1231f 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp @@ -34,6 +34,7 @@ struct smart_ref { struct OtherType { ~OtherType(); + void constMethod() const; }; template struct SomeComplexTemplate { @@ -89,6 +90,7 @@ void positiveOtherType() { const auto O = getOtherType(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'O' is copy-constructed from a const reference; consider making it a const reference [performance-unnecessary-copy-initialization] // CHECK-FIXES: const auto& O = getOtherType(); + O.constMethod(); } void negativeNotTooComplexRef() { 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 8cc095e431fd..3a4c3ec869e1 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 @@ -38,60 +38,88 @@ void PositiveFunctionCall() { const auto AutoAssigned = ExpensiveTypeReference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference; consider making it a const reference [performance-unnecessary-copy-initialization] // CHECK-FIXES: const auto& AutoAssigned = ExpensiveTypeReference(); + AutoAssigned.constMethod(); + const auto AutoCopyConstructed(ExpensiveTypeReference()); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed' // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference()); + AutoCopyConstructed.constMethod(); + const ExpensiveToCopyType VarAssigned = ExpensiveTypeReference(); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned' // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = ExpensiveTypeReference(); + VarAssigned.constMethod(); + const ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference()); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed' // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(ExpensiveTypeReference()); + VarCopyConstructed.constMethod(); } void PositiveMethodCallConstReferenceParam(const ExpensiveToCopyType &Obj) { const auto AutoAssigned = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = Obj.reference(); + AutoAssigned.constMethod(); + const auto AutoCopyConstructed(Obj.reference()); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed' // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference()); + AutoCopyConstructed.constMethod(); + const ExpensiveToCopyType VarAssigned = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned' // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference(); + VarAssigned.constMethod(); + const ExpensiveToCopyType VarCopyConstructed(Obj.reference()); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed' // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference()); + VarCopyConstructed.constMethod(); } void PositiveMethodCallConstParam(const ExpensiveToCopyType Obj) { const auto AutoAssigned = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = Obj.reference(); + AutoAssigned.constMethod(); + const auto AutoCopyConstructed(Obj.reference()); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed' // CHECK-FIXES: const auto& AutoCopyConstructed(Obj.reference()); + AutoCopyConstructed.constMethod(); + const ExpensiveToCopyType VarAssigned = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned' // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj.reference(); + VarAssigned.constMethod(); + const ExpensiveToCopyType VarCopyConstructed(Obj.reference()); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed' // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj.reference()); + VarCopyConstructed.constMethod(); } void PositiveMethodCallConstPointerParam(const ExpensiveToCopyType *const Obj) { const auto AutoAssigned = Obj->reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = Obj->reference(); + AutoAssigned.constMethod(); + const auto AutoCopyConstructed(Obj->reference()); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed' // CHECK-FIXES: const auto& AutoCopyConstructed(Obj->reference()); + AutoCopyConstructed.constMethod(); + const ExpensiveToCopyType VarAssigned = Obj->reference(); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned' // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = Obj->reference(); + VarAssigned.constMethod(); + const ExpensiveToCopyType VarCopyConstructed(Obj->reference()); // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed' // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(Obj->reference()); + VarCopyConstructed.constMethod(); } void PositiveLocalConstValue() { @@ -99,6 +127,7 @@ void PositiveLocalConstValue() { const auto UnnecessaryCopy = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy' // CHECK-FIXES: const auto& UnnecessaryCopy = Obj.reference(); + UnnecessaryCopy.constMethod(); } void PositiveLocalConstRef() { @@ -107,6 +136,7 @@ void PositiveLocalConstRef() { const auto UnnecessaryCopy = ConstReference.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy' // CHECK-FIXES: const auto& UnnecessaryCopy = ConstReference.reference(); + UnnecessaryCopy.constMethod(); } void PositiveLocalConstPointer() { @@ -115,6 +145,7 @@ void PositiveLocalConstPointer() { const auto UnnecessaryCopy = ConstPointer->reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'UnnecessaryCopy' // CHECK-FIXES: const auto& UnnecessaryCopy = ConstPointer->reference(); + UnnecessaryCopy.constMethod(); } void NegativeFunctionCallTrivialType() { @@ -132,15 +163,22 @@ void PositiveFunctionCallExpensiveTypeNonConstVariable() { auto AutoAssigned = ExpensiveTypeReference(); // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'AutoAssigned' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization] // CHECK-FIXES: const auto& AutoAssigned = ExpensiveTypeReference(); + AutoAssigned.constMethod(); + auto AutoCopyConstructed(ExpensiveTypeReference()); // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'AutoCopyConstructed' // CHECK-FIXES: const auto& AutoCopyConstructed(ExpensiveTypeReference()); + AutoCopyConstructed.constMethod(); + ExpensiveToCopyType VarAssigned = ExpensiveTypeReference(); // CHECK-MESSAGES: [[@LINE-1]]:23: warning: the variable 'VarAssigned' // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = ExpensiveTypeReference(); + VarAssigned.constMethod(); + ExpensiveToCopyType VarCopyConstructed(ExpensiveTypeReference()); // CHECK-MESSAGES: [[@LINE-1]]:23: warning: the variable 'VarCopyConstructed' // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(ExpensiveTypeReference()); + VarCopyConstructed.constMethod(); } void positiveNonConstVarInCodeBlock(const ExpensiveToCopyType &Obj) { @@ -181,6 +219,7 @@ void PositiveMethodCallNonConstRefNotModified(ExpensiveToCopyType &Obj) { const auto AutoAssigned = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = Obj.reference(); + AutoAssigned.constMethod(); } void NegativeMethodCallNonConstRefIsModified(ExpensiveToCopyType &Obj) { @@ -195,6 +234,7 @@ void PositiveMethodCallNonConstNotModified(ExpensiveToCopyType Obj) { const auto AutoAssigned = Obj.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = Obj.reference(); + AutoAssigned.constMethod(); } void NegativeMethodCallNonConstValueArgumentIsModified(ExpensiveToCopyType Obj) { @@ -207,6 +247,7 @@ void PositiveMethodCallNonConstPointerNotModified(ExpensiveToCopyType *const Obj // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = Obj->reference(); Obj->constMethod(); + AutoAssigned.constMethod(); } void NegativeMethodCallNonConstPointerIsModified(ExpensiveToCopyType *const Obj) { @@ -222,6 +263,7 @@ void PositiveLocalVarIsNotModified() { const auto AutoAssigned = LocalVar.reference(); // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' // CHECK-FIXES: const auto& AutoAssigned = LocalVar.reference(); + AutoAssigned.constMethod(); } void NegativeLocalVarIsModified() { @@ -252,6 +294,7 @@ void PositiveMacroArgument(const ExpensiveToCopyType &Obj) { // CHECK-MESSAGES: [[@LINE-1]]:48: warning: the variable 'CopyInMacroArg' is copy-constructed // Ensure fix is not applied. // CHECK-FIXES: auto CopyInMacroArg = Obj.reference() + CopyInMacroArg.constMethod(); } void PositiveLocalCopyConstMethodInvoked() { @@ -285,6 +328,7 @@ void PositiveLocalCopyUsedAsConstRef() { // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy_4' // CHECK-FIXES: const ExpensiveToCopyType& copy_4 = orig; useAsConstReference(orig); + copy_4.constMethod(); } void PositiveLocalCopyTwice() { @@ -370,6 +414,7 @@ void WarningOnlyMultiDeclStmt() { ExpensiveToCopyType copy = orig, copy2; // CHECK-MESSAGES: [[@LINE-1]]:23: warning: local copy 'copy' of the variable 'orig' is never modified; consider avoiding the copy [performance-unnecessary-copy-initialization] // CHECK-FIXES: ExpensiveToCopyType copy = orig, copy2; + copy.constMethod(); } class Element {}; @@ -450,6 +495,7 @@ template struct function { // Custom copy constructor makes it expensive to copy; function(const function &); + void constMethod() const; }; } // namespace std @@ -457,6 +503,7 @@ void positiveFakeStdFunction(std::function F) { auto Copy = F; // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'Copy' of the variable 'F' is never modified; // CHECK-FIXES: const auto& Copy = F; + Copy.constMethod(); } } // namespace fake @@ -491,6 +538,7 @@ void positiveCopiedFromReferenceToConstVar() { // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'UnnecessaryCopy' of // CHECK-FIXES: const auto& UnnecessaryCopy = Ref; Orig.constMethod(); + UnnecessaryCopy.constMethod(); } void negativeCopiedFromGetterOfReferenceToModifiedVar() { @@ -507,4 +555,17 @@ void positiveCopiedFromGetterOfReferenceToConstVar() { // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'UnnecessaryCopy' is // CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference(); Orig.constMethod(); + UnnecessaryCopy.constMethod(); +} + +void positiveUnusedReferenceIsRemoved() { + // clang-format off + const auto AutoAssigned = ExpensiveTypeReference(); int i = 0; // Foo bar. + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned' is copy-constructed from a const reference but is never used; consider removing the statement [performance-unnecessary-copy-initialization] + // CHECK-FIXES-NOT: const auto AutoAssigned = ExpensiveTypeReference(); + // CHECK-FIXES: int i = 0; // Foo bar. + auto TrailingCommentRemoved = ExpensiveTypeReference(); // Trailing comment. + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'TrailingCommentRemoved' is copy-constructed from a const reference but is never used; + // CHECK-FIXES-NOT: auto TrailingCommentRemoved = ExpensiveTypeReference(); // Trailing comment. + // clang-format on }