[clang-tidy] Fix false in unnecessary-value-param inside templates #98488
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-tidy Author: Dmitry Polukhin (dmpolukhin) ChangesSummary: Test Plan: check-clang-tools Full diff: https://github.com/llvm/llvm-project/pull/98488.diff 2 Files Affected:
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); }`
|
|
@llvm/pr-subscribers-clang-analysis Author: Dmitry Polukhin (dmpolukhin) ChangesSummary: Test Plan: check-clang-tools Full diff: https://github.com/llvm/llvm-project/pull/98488.diff 2 Files Affected:
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); }`
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
PiotrZSL
reviewed
Jul 15, 2024
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…98488) 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 Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250876
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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