[clang] [clang-tools-extra] [clang-tidy] Fix false in unnecessary-value-param inside templates (PR #98488)
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 15 15:01:48 PDT 2024
https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/98488
>From a05c4ca2c2e61653e7bd8d3a2f5faa6b86daa615 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Thu, 11 Jul 2024 07:01:49 -0700
Subject: [PATCH 1/4] [clang-tidy] Fix false in unnecessary-value-param inside
templates
Summary:
If callExpr is type dependent, there is no way to analyze individual arguments
until template specialization. Before this diff only calls with dependent
callees were skipped so unnecessary-value-param was processing arguments that
had non-dependent type that gave false positives because the call was not fully
resolved till specialization. So now instead of checking type dependent callee,
the whole expression will be checked for type dependent.
Test Plan: check-clang-tools
---
.../performance/unnecessary-value-param.cpp | 34 +++++++++++++++++++
clang/lib/Analysis/ExprMutationAnalyzer.cpp | 9 +++--
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
index d578eedd94a39..b1696ad3c3d59 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
@@ -2,6 +2,31 @@
// CHECK-FIXES: #include <utility>
+namespace std {
+template <typename>
+struct remove_reference;
+
+template <typename _Tp>
+struct remove_reference {
+ typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &> {
+ typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &&> {
+ typedef _Tp type;
+};
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+ return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
+}
+} // namespace std
+
struct ExpensiveToCopyType {
const ExpensiveToCopyType & constReference() const {
return *this;
@@ -402,3 +427,12 @@ int templateSpecializationFunction(ExpensiveToCopyType E) {
// CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
return 0;
}
+
+struct B {
+ static void bar(ExpensiveMovableType a, ExpensiveMovableType b);
+};
+
+template <typename T>
+void NegativeCallWithDependentAndNondependentArgs(ExpensiveMovableType a, T b) {
+ B::bar(std::move(a), b);
+}
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 3b3782fa1db9a..70a1d7b52ffe9 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -404,15 +404,14 @@ ExprMutationAnalyzer::Analyzer::findDirectMutation(const Expr *Exp) {
memberExpr(hasObjectExpression(canResolveToExpr(Exp)))),
nonConstReferenceType());
const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
- const auto TypeDependentCallee =
- callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),
- cxxDependentScopeMemberExpr(),
- hasType(templateTypeParmType()), isTypeDependent())));
const auto AsNonConstRefArg = anyOf(
callExpr(NonConstRefParam, NotInstantiated),
cxxConstructExpr(NonConstRefParam, NotInstantiated),
- callExpr(TypeDependentCallee, hasAnyArgument(canResolveToExpr(Exp))),
+ // If the call is type-dependent, we can't properly process any
+ // argument because required type conversions and implicit casts
+ // will be inserted only after specialization.
+ callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
// Previous False Positive in the following Code:
// `template <typename T> void f() { int i = 42; new Type<T>(i); }`
>From dbcdb0655318b7ba674cf00c7a9da9917a056102 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Thu, 11 Jul 2024 07:35:21 -0700
Subject: [PATCH 2/4] Apply git-clang-format
---
clang/lib/Analysis/ExprMutationAnalyzer.cpp | 34 ++++++++++-----------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 70a1d7b52ffe9..6d726ae44104e 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -405,23 +405,23 @@ ExprMutationAnalyzer::Analyzer::findDirectMutation(const Expr *Exp) {
nonConstReferenceType());
const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
- const auto AsNonConstRefArg = anyOf(
- callExpr(NonConstRefParam, NotInstantiated),
- cxxConstructExpr(NonConstRefParam, NotInstantiated),
- // If the call is type-dependent, we can't properly process any
- // argument because required type conversions and implicit casts
- // will be inserted only after specialization.
- callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
- cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
- // Previous False Positive in the following Code:
- // `template <typename T> void f() { int i = 42; new Type<T>(i); }`
- // Where the constructor of `Type` takes its argument as reference.
- // The AST does not resolve in a `cxxConstructExpr` because it is
- // type-dependent.
- parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
- // If the initializer is for a reference type, there is no cast for
- // the variable. Values are cast to RValue first.
- initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
+ const auto AsNonConstRefArg =
+ anyOf(callExpr(NonConstRefParam, NotInstantiated),
+ cxxConstructExpr(NonConstRefParam, NotInstantiated),
+ // If the call is type-dependent, we can't properly process any
+ // argument because required type conversions and implicit casts
+ // will be inserted only after specialization.
+ callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
+ cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
+ // Previous False Positive in the following Code:
+ // `template <typename T> void f() { int i = 42; new Type<T>(i); }`
+ // Where the constructor of `Type` takes its argument as reference.
+ // The AST does not resolve in a `cxxConstructExpr` because it is
+ // type-dependent.
+ parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
+ // If the initializer is for a reference type, there is no cast for
+ // the variable. Values are cast to RValue first.
+ initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
// Captured by a lambda by reference.
// If we're initializing a capture with 'Exp' directly then we're initializing
>From a5c3353aa9f98c56deedd341b195f0706bdc1f83 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <34227995+dmpolukhin at users.noreply.github.com>
Date: Mon, 15 Jul 2024 21:57:51 +0100
Subject: [PATCH 3/4] Fix unnecessary-value-param.cpp to include only real
changes
---
.../performance/unnecessary-value-param.cpp | 34 +------------------
1 file changed, 1 insertion(+), 33 deletions(-)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
index 661d02e5f3ccd..7c7ae43698929 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
@@ -383,38 +383,6 @@ void fun() {
NegativeUsingConstructor S(E);
}
-template<typename T>
-void templateFunction(T) {
-}
-
-template<>
-void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
- // CHECK-MESSAGES: [[@LINE-1]]:64: warning: the parameter 'E' is copied
- // CHECK-FIXES: void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
- E.constReference();
-}
-
-template <class T>
-T templateSpecializationFunction(ExpensiveToCopyType E) {
- // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
- // CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
- return T();
-}
-
-template <>
-bool templateSpecializationFunction(ExpensiveToCopyType E) {
- // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
- // CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
- return true;
-}
-
-template <>
-int templateSpecializationFunction(ExpensiveToCopyType E) {
- // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
- // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
- return 0;
-}
-
struct B {
static void bar(ExpensiveMovableType a, ExpensiveMovableType b);
};
@@ -422,4 +390,4 @@ struct B {
template <typename T>
void NegativeCallWithDependentAndNondependentArgs(ExpensiveMovableType a, T b) {
B::bar(std::move(a), b);
-}
\ No newline at end of file
+}
>From f5621fc6549de65d0b246d9ed6cc5b3dcf028c1f Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Mon, 15 Jul 2024 15:00:24 -0700
Subject: [PATCH 4/4] Updated release notes entry
---
clang-tools-extra/docs/ReleaseNotes.rst | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 004811d2eca4f..0e12e80d9f393 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -446,7 +446,8 @@ Changes in existing checks
<clang-tidy/checks/performance/unnecessary-value-param>` check
detecting more cases for template functions including lambdas with ``auto``.
E.g., ``std::sort(a.begin(), a.end(), [](auto x, auto y) { return a > b; });``
- will be detected for expensive to copy types.
+ will be detected for expensive to copy types. Fixed false positives for
+ dependent call expressions.
- Improved :doc:`readability-avoid-return-with-void-value
<clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding
More information about the cfe-commits
mailing list