flx created this revision.
flx added reviewers: alexfh, sbenza.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
flx requested review of this revision.
This restriction avoids cases where an alias is returned to an argument and
which could lead to to a false positive change. Example:
struct S {
S(const S&);
void modify();
};
const S& alias(const S& a) {
return a;
}
void f(S a) {
const S c = alias(a);
a.modify();
}
Taking a const reference of the return value of alias would not be safe since a
is modified.
Still allow methods and functions with default arguments if they are called
with their default arguments.
A more involved alternative approach would be to inspect each argument passed
to these functions for its constness, but this can get complex quickly.
As I'm writing this I'm realizing though that this precludes catching repeated
protobuff messages:
Message m;
const auto field = m.repeated_field(0);
Maybe this approach should be refined to allowing functions and methods that
take all parameters by value?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D87455
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
@@ -6,6 +6,8 @@
const ExpensiveToCopyType &reference() const;
void nonConstMethod();
bool constMethod() const;
+ const ExpensiveToCopyType &methodWithArg(const ExpensiveToCopyType &) const;
+ const ExpensiveToCopyType &methodWithDefaultArg(int arg = 0) const;
};
struct TrivialToCopyType {
@@ -23,6 +25,9 @@
ExpensiveToCopyType global_expensive_to_copy_type;
const ExpensiveToCopyType &ExpensiveTypeReference();
+const ExpensiveToCopyType &freeFunctionWithArg(const ExpensiveToCopyType &);
+const ExpensiveToCopyType &freeFunctionWithDefaultArg(
+ const ExpensiveToCopyType *arg = nullptr);
const TrivialToCopyType &TrivialTypeReference();
void mutate(ExpensiveToCopyType &);
@@ -387,3 +392,38 @@
for (const Element &E : Container()) {
}
}
+
+// This should not trigger the check as the argument could introduce an alias.
+void negativeInitializedFromMethodWithArg() {
+ ExpensiveToCopyType Orig;
+ const ExpensiveToCopyType Copy = Orig.methodWithArg(Orig);
+}
+
+// This should not trigger the check as the argument could introduce an alias.
+void negativeInitializedFromFreeFunctionWithArg() {
+ ExpensiveToCopyType Orig;
+ const ExpensiveToCopyType Copy = freeFunctionWithArg(Orig);
+}
+
+void positiveInitializedFromMethodWithDefaultArg() {
+ ExpensiveToCopyType Orig;
+ const ExpensiveToCopyType Copy = Orig.methodWithDefaultArg();
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'Copy'
+ // CHECK-FIXES: const ExpensiveToCopyType& Copy = Orig.methodWithDefaultArg();
+}
+
+void negativeInitializedFromMethodWithNonDefaultArg() {
+ ExpensiveToCopyType Orig;
+ const ExpensiveToCopyType Copy = Orig.methodWithDefaultArg(5);
+}
+
+void positiveInitializedFromFreeFunctionWithDefaultArg() {
+ const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg();
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'Copy'
+ // CHECK-FIXES: const ExpensiveToCopyType& Copy = freeFunctionWithDefaultArg();
+}
+
+void negativeInitialzedFromFreeFunctionWithNonDefaultArg() {
+ ExpensiveToCopyType Orig;
+ const ExpensiveToCopyType Copy = freeFunctionWithDefaultArg(&Orig);
+}
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
@@ -36,6 +36,7 @@
private:
void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
bool IssueFix, const VarDecl *ObjectArg,
+ const CallExpr *InitCallExpr,
ASTContext &Context);
void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
const Stmt &BlockStmt, bool IssueFix,
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
@@ -29,6 +29,14 @@
}
}
+template <typename Call>
+bool hasNonDefaultArgs(const Call &CallExpr, unsigned int StartIndex = 0) {
+ for (unsigned int I = StartIndex; I < CallExpr.getNumArgs(); ++I)
+ if (!CallExpr.getArg(I)->isDefaultArgument())
+ return true;
+ return false;
+}
+
} // namespace
using namespace ::clang::ast_matchers;
@@ -51,10 +59,12 @@
// called object.
auto ConstRefReturningMethodCall =
cxxMemberCallExpr(callee(cxxMethodDecl(returns(ConstReference))),
- on(declRefExpr(to(varDecl().bind("objectArg")))));
+ on(declRefExpr(to(varDecl().bind("objectArg")))))
+ .bind("initMethodCall");
auto ConstRefReturningFunctionCall =
callExpr(callee(functionDecl(returns(ConstReference))),
- unless(callee(cxxMethodDecl())));
+ unless(callee(cxxMethodDecl())))
+ .bind("initFunctionCall");
auto localVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
return compoundStmt(
@@ -96,6 +106,10 @@
const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg");
const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
+ const auto *InitFunctionCall =
+ Result.Nodes.getNodeAs<CallExpr>("initFunctionCall");
+ const auto *InitMethodCall =
+ Result.Nodes.getNodeAs<CallExpr>("initMethodCall");
TraversalKindScope RAII(*Result.Context, ast_type_traits::TK_AsIs);
@@ -108,12 +122,13 @@
// 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
// the first.
- for (unsigned int i = 1; i < CtorCall->getNumArgs(); ++i)
- if (!CtorCall->getArg(i)->isDefaultArgument())
- return;
+ if (hasNonDefaultArgs(*CtorCall, 1))
+ return;
if (OldVar == nullptr) {
handleCopyFromMethodReturn(*NewVar, *BlockStmt, IssueFix, ObjectArg,
+ InitMethodCall != nullptr ? InitMethodCall
+ : InitFunctionCall,
*Result.Context);
} else {
handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, IssueFix,
@@ -123,14 +138,15 @@
void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix,
- const VarDecl *ObjectArg, ASTContext &Context) {
+ const VarDecl *ObjectArg, const CallExpr *InitCallExpr,
+ ASTContext &Context) {
bool IsConstQualified = Var.getType().isConstQualified();
- if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
+ if (hasNonDefaultArgs(*InitCallExpr) ||
+ (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context)))
return;
if (ObjectArg != nullptr &&
!isOnlyUsedAsConst(*ObjectArg, BlockStmt, Context))
return;
-
auto Diagnostic =
diag(Var.getLocation(),
IsConstQualified ? "the const qualified variable %0 is "
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
