This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5dbe3bf4b8db: [clang-tidy]
performance-unnecessary-copy-initialization: Remove the complete… (authored by
flx).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102175/new/
https://reviews.llvm.org/D102175
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-allowed-types.cpp
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
@@ -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
}
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp
+++ 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 <typename T> 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() {
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 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<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
@@ -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<VarDecl>(ObjectArgId);
const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
+ const auto *Stmt = Result.Nodes.getNodeAs<DeclStmt>("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>("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(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
