diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h +++ b/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 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/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp --- 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 @@ 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 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 @@ 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 @@ 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 @@ 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/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 --- 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 OtherType { ~OtherType(); + void constMethod() const; }; template struct SomeComplexTemplate { @@ -89,6 +90,7 @@ 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 --- 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ // 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 @@ 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 @@ // 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 @@ // 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 @@ 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 @@ struct function { // Custom copy constructor makes it expensive to copy; function(const function &); + void constMethod() const; }; } // namespace std @@ -457,6 +503,7 @@ 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 @@ // 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 @@ // 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 }