maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[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
This commit is contained in:
Felix Berger 2021-05-10 11:26:00 -04:00
parent 68d0db0b6d
commit 5dbe3bf4b8
4 changed files with 130 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

87
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
View File

@ -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<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 @@ 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(

7
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
View File

@ -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<std::string> AllowedTypes;
};

2
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-allowed-types.cpp
View File

@ -34,6 +34,7 @@ struct smart_ref {
struct OtherType {
~OtherType();
void constMethod() const;
};
template <typename T> 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() {

61
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
View File

@ -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 <class R, class... Args>
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<void(int)> 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
}