Skip to content

Commit 1ce8989

Browse files
authored
[clang-tidy] Fix false in unnecessary-value-param inside templates (#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
1 parent b1864a8 commit 1ce8989

File tree

3 files changed

+54
-20
lines changed

3 files changed

+54
-20
lines changed

clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,8 @@ Changes in existing checks
450450
<clang-tidy/checks/performance/unnecessary-value-param>` check
451451
detecting more cases for template functions including lambdas with ``auto``.
452452
E.g., ``std::sort(a.begin(), a.end(), [](auto x, auto y) { return a > b; });``
453-
will be detected for expensive to copy types.
453+
will be detected for expensive to copy types. Fixed false positives for
454+
dependent call expressions.
454455

455456
- Improved :doc:`readability-avoid-return-with-void-value
456457
<clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding

clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,31 @@
22

33
// CHECK-FIXES: #include <utility>
44

5+
namespace std {
6+
template <typename>
7+
struct remove_reference;
8+
9+
template <typename _Tp>
10+
struct remove_reference {
11+
typedef _Tp type;
12+
};
13+
14+
template <typename _Tp>
15+
struct remove_reference<_Tp &> {
16+
typedef _Tp type;
17+
};
18+
19+
template <typename _Tp>
20+
struct remove_reference<_Tp &&> {
21+
typedef _Tp type;
22+
};
23+
24+
template <typename _Tp>
25+
constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
26+
return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
27+
}
28+
} // namespace std
29+
530
struct ExpensiveToCopyType {
631
const ExpensiveToCopyType & constReference() const {
732
return *this;
@@ -357,3 +382,12 @@ void fun() {
357382
ExpensiveToCopyType E;
358383
NegativeUsingConstructor S(E);
359384
}
385+
386+
struct B {
387+
static void bar(ExpensiveMovableType a, ExpensiveMovableType b);
388+
};
389+
390+
template <typename T>
391+
void NegativeCallWithDependentAndNondependentArgs(ExpensiveMovableType a, T b) {
392+
B::bar(std::move(a), b);
393+
}

clang/lib/Analysis/ExprMutationAnalyzer.cpp

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -404,25 +404,24 @@ ExprMutationAnalyzer::Analyzer::findDirectMutation(const Expr *Exp) {
404404
memberExpr(hasObjectExpression(canResolveToExpr(Exp)))),
405405
nonConstReferenceType());
406406
const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
407-
const auto TypeDependentCallee =
408-
callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),
409-
cxxDependentScopeMemberExpr(),
410-
hasType(templateTypeParmType()), isTypeDependent())));
411-
412-
const auto AsNonConstRefArg = anyOf(
413-
callExpr(NonConstRefParam, NotInstantiated),
414-
cxxConstructExpr(NonConstRefParam, NotInstantiated),
415-
callExpr(TypeDependentCallee, hasAnyArgument(canResolveToExpr(Exp))),
416-
cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
417-
// Previous False Positive in the following Code:
418-
// `template <typename T> void f() { int i = 42; new Type<T>(i); }`
419-
// Where the constructor of `Type` takes its argument as reference.
420-
// The AST does not resolve in a `cxxConstructExpr` because it is
421-
// type-dependent.
422-
parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
423-
// If the initializer is for a reference type, there is no cast for
424-
// the variable. Values are cast to RValue first.
425-
initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
407+
408+
const auto AsNonConstRefArg =
409+
anyOf(callExpr(NonConstRefParam, NotInstantiated),
410+
cxxConstructExpr(NonConstRefParam, NotInstantiated),
411+
// If the call is type-dependent, we can't properly process any
412+
// argument because required type conversions and implicit casts
413+
// will be inserted only after specialization.
414+
callExpr(isTypeDependent(), hasAnyArgument(canResolveToExpr(Exp))),
415+
cxxUnresolvedConstructExpr(hasAnyArgument(canResolveToExpr(Exp))),
416+
// Previous False Positive in the following Code:
417+
// `template <typename T> void f() { int i = 42; new Type<T>(i); }`
418+
// Where the constructor of `Type` takes its argument as reference.
419+
// The AST does not resolve in a `cxxConstructExpr` because it is
420+
// type-dependent.
421+
parenListExpr(hasDescendant(expr(canResolveToExpr(Exp)))),
422+
// If the initializer is for a reference type, there is no cast for
423+
// the variable. Values are cast to RValue first.
424+
initListExpr(hasAnyInit(expr(canResolveToExpr(Exp)))));
426425

427426
// Captured by a lambda by reference.
428427
// If we're initializing a capture with 'Exp' directly then we're initializing

0 commit comments

Comments
 (0)