[clang-tools-extra] [clang-tidy] Add `find_if` -> `upper_bound`/`lower_bound` to `performance-inefficient-algorithm` (PR #218005)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 21 12:10:12 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: David Meng (davidmenggx)
<details>
<summary>Changes</summary>
A `std::find_if` that looks for the first element past a fixed bound scans an ordered set linearly where binary search would do it in logarithmic time.
```cpp
std::find_if(s.begin(), s.end(), [&](int v) { return v > i; })
// -> s.upper_bound(i)
```
Cases covered:
- `std::set` and `std::multiset` ordered by `std::less` (including the transparent `std::less<void>`), held by value, reference or pointer.
- A lambda written at the call site whose body is a single `return` of a built-in comparison.
- Integral, enumeration and pointer keys.
- A bound whose type differs from the key, when the conversion provably cannot change its value.
Cases deliberately avoided:
- Maps, unordered containers, and comparators other than `std::less`.
- Overloaded comparison operators.
- Floating-point keys.
- Signed/unsigned mixes in which the usual arithmetic conversions order the elements differently than the container does.
- Generic lambdas and lambdas with explicit template parameters, extra parameters, or init-captures, and predicates that are not a lambda at the call site.
- Bounds that name the element, are volatile, or have side effects (since the fix evaluates the bound once instead of once per element).
Closes https://github.com/llvm/llvm-project/issues/216380
---
Patch is 27.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/218005.diff
6 Files Affected:
- (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp (+2-6)
- (modified) clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp (+160-27)
- (modified) clang-tools-extra/clang-tidy/utils/Matchers.h (+6)
- (modified) clang-tools-extra/docs/ReleaseNotes.md (+12-6)
- (modified) clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-algorithm.md (+15)
- (modified) clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp (+204-4)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
index 161e17c47b1f5..33e17a4872c14 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "OwningMemoryCheck.h"
+#include "../utils/Matchers.h"
#include "../utils/OptionsUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
@@ -18,11 +19,6 @@ using namespace clang::ast_matchers::internal;
namespace clang::tidy::cppcoreguidelines {
namespace {
-AST_MATCHER_P(LambdaExpr, hasCallOperator, Matcher<CXXMethodDecl>,
- InnerMatcher) {
- return InnerMatcher.matches(*Node.getCallOperator(), Finder, Builder);
-}
-
AST_MATCHER_P(LambdaExpr, hasLambdaBody, Matcher<Stmt>, InnerMatcher) {
return InnerMatcher.matches(*Node.getBody(), Finder, Builder);
}
@@ -197,7 +193,7 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) {
hasAncestor(decl(equalsBoundNode("context"),
equalsBoundNode("scope-decl"))))
.bind("bad_owner_return")))),
- hasCallOperator(returns(
+ matchers::hasCallOperator(returns(
qualType(unless(hasDeclaration(OwnerDecl))).bind("result"))))
.bind("lambda"),
this);
diff --git a/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp
index aa3c6fa5c1dab..bff9ec6c34e9b 100644
--- a/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp
@@ -7,7 +7,9 @@
//===----------------------------------------------------------------------===//
#include "InefficientAlgorithmCheck.h"
+#include "../utils/Matchers.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
@@ -24,6 +26,60 @@ static bool areTypesCompatible(QualType Left, QualType Right) {
Right->getCanonicalTypeUnqualified();
}
+/// Returns true if built-in `<` orders `T` the way `std::less<T>` does,
+/// excluding floating point (NaN compares unordered).
+static bool hasBuiltinOrder(QualType T) {
+ return T->isIntegralOrEnumerationType() || T->isPointerType();
+}
+
+/// Returns true if converting `Bound` to `KeyType` cannot change its value.
+static bool boundConvertsExactly(const Expr *Bound, QualType KeyType,
+ const ASTContext &Ctx) {
+ if (!KeyType->isIntegralType(Ctx))
+ return false;
+ const bool KeySigned = KeyType->isSignedIntegerOrEnumerationType();
+
+ const QualType BoundType = Bound->getType();
+ if (BoundType->isIntegerType()) {
+ const bool BoundSigned = BoundType->isSignedIntegerOrEnumerationType();
+ const unsigned BoundWidth = Ctx.getIntWidth(BoundType);
+ const unsigned KeyWidth = Ctx.getIntWidth(KeyType);
+ if ((BoundSigned == KeySigned) ? (BoundWidth <= KeyWidth)
+ : (!BoundSigned && BoundWidth < KeyWidth))
+ return true;
+ }
+
+ // Otherwise the bound must be a constant that survives the conversion.
+ Expr::EvalResult Eval;
+ if (!Bound->EvaluateAsInt(Eval, Ctx))
+ return false;
+ const llvm::APSInt Value = Eval.Val.getInt();
+ llvm::APSInt Converted = Value.extOrTrunc(Ctx.getIntWidth(KeyType));
+ Converted.setIsSigned(KeySigned);
+ return llvm::APSInt::isSameValue(Converted, Value);
+}
+
+/// Matches a call taking `c.begin()` and `c.end()` as its first two arguments,
+/// where `c` is a `Container`. Binds `c` as "IneffContExpr", its
+/// declaration as "IneffContObj" and its class as "IneffCont", or as
+/// "IneffContPtr" when `c` is a pointer.
+static ast_matchers::internal::Matcher<CallExpr> hasWholeContainerRange(
+ const ast_matchers::internal::BindableMatcher<Decl> &Container) {
+ return callExpr(
+ hasArgument(
+ 0, cxxMemberCallExpr(
+ callee(cxxMethodDecl(hasName("begin"))),
+ on(declRefExpr(hasDeclaration(decl().bind("IneffContObj")),
+ anyOf(hasType(Container.bind("IneffCont")),
+ hasType(pointsTo(
+ Container.bind("IneffContPtr")))))
+ .bind("IneffContExpr")))),
+ hasArgument(1,
+ cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"))),
+ on(declRefExpr(hasDeclaration(
+ equalsBoundNode("IneffContObj")))))));
+}
+
void InefficientAlgorithmCheck::registerMatchers(MatchFinder *Finder) {
const auto Algorithms =
hasAnyName("::std::find", "::std::count", "::std::equal_range",
@@ -33,29 +89,68 @@ void InefficientAlgorithmCheck::registerMatchers(MatchFinder *Finder) {
"::std::unordered_set", "::std::unordered_map",
"::std::unordered_multiset", "::std::unordered_multimap"));
- const auto Matcher =
- callExpr(
- callee(functionDecl(Algorithms)), argumentCountAtLeast(3),
- hasArgument(
- 0, cxxMemberCallExpr(
- callee(cxxMethodDecl(hasName("begin"))),
- on(declRefExpr(
- hasDeclaration(decl().bind("IneffContObj")),
- anyOf(hasType(ContainerMatcher.bind("IneffCont")),
- hasType(pointsTo(
- ContainerMatcher.bind("IneffContPtr")))))
- .bind("IneffContExpr")))),
- hasArgument(
- 1, cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"))),
- on(declRefExpr(hasDeclaration(
- equalsBoundNode("IneffContObj")))))))
- .bind("IneffAlg");
-
- Finder->addMatcher(Matcher, this);
+ Finder->addMatcher(callExpr(callee(functionDecl(Algorithms)),
+ argumentCountAtLeast(3),
+ hasWholeContainerRange(ContainerMatcher))
+ .bind("IneffAlg"),
+ this);
+
+ // `upper_bound` (for `>`) and `lower_bound` (for `>=`) binary search where
+ // `std::find_if` linearly scans. Only in containers ordered by `std::less`.
+ const auto SortedContainer = classTemplateSpecializationDecl(
+ hasAnyName("::std::set", "::std::multiset"),
+ hasTemplateArgument(
+ 1, refersToType(hasDeclaration(
+ classTemplateSpecializationDecl(hasName("::std::less"))))));
+ const auto RefersToElement =
+ declRefExpr(to(parmVarDecl(equalsBoundNode("PredElement"))));
+ // The bound is lifted out of the predicate, so it must not name the element.
+ const auto Bound = expr(unless(findAll(RefersToElement))).bind("Bound");
+ const auto BoundComparison =
+ binaryOperator(
+ anyOf(allOf(hasAnyOperatorName(">", ">="),
+ hasLHS(ignoringParenImpCasts(RefersToElement)),
+ hasRHS(ignoringParenImpCasts(Bound))),
+ allOf(hasAnyOperatorName("<", "<="),
+ hasLHS(ignoringParenImpCasts(Bound)),
+ hasRHS(ignoringParenImpCasts(RefersToElement)))))
+ .bind("PredOp");
+ // A second parameter or an init-capture would let the bound name a
+ // declaration that is not in scope at the call site the fix moves it to.
+ const auto BoundPredicate =
+ lambdaExpr(
+ matchers::hasCallOperator(cxxMethodDecl(
+ parameterCountIs(1),
+ hasParameter(0, parmVarDecl().bind("PredElement")))),
+ unless(hasAnyCapture(capturesVar(varDecl(isInitCapture())))),
+ has(compoundStmt(statementCountIs(1),
+ hasAnySubstatement(returnStmt(hasReturnValue(
+ ignoringParenImpCasts(BoundComparison)))))))
+ .bind("Pred");
+
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::std::find_if"))),
+ hasWholeContainerRange(SortedContainer),
+ hasArgument(2, ignoringElidableConstructorCall(BoundPredicate)))
+ .bind("IneffAlg"),
+ this);
}
void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) {
const auto *AlgCall = Result.Nodes.getNodeAs<CallExpr>("IneffAlg");
+ const auto *PredOp = Result.Nodes.getNodeAs<BinaryOperator>("PredOp");
+
+ const Expr *ValueExpr = AlgCall->getArg(2);
+ const Expr *ValueExprAsWritten = ValueExpr;
+ const Expr *ElementExpr = nullptr;
+ if (PredOp) {
+ const BinaryOperatorKind Opcode = PredOp->getOpcode();
+ const bool ElementOnLeft = Opcode == BO_GT || Opcode == BO_GE;
+ ValueExpr = Result.Nodes.getNodeAs<Expr>("Bound");
+ ValueExprAsWritten = ElementOnLeft ? PredOp->getRHS() : PredOp->getLHS();
+ ElementExpr = ElementOnLeft ? PredOp->getLHS() : PredOp->getRHS();
+ }
+
const auto *IneffCont =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("IneffCont");
bool PtrToContainer = false;
@@ -68,12 +163,42 @@ void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) {
const bool Unordered = IneffContName.contains("unordered");
const bool Maplike = IneffContName.contains("map");
- // Store if the key type of the container is compatible with the value
- // that is searched for.
- const QualType ValueType = AlgCall->getArg(2)->getType();
+ const QualType ValueType = ValueExpr->getType();
const QualType KeyType =
IneffCont->getTemplateArgs()[0].getAsType().getCanonicalType();
- const bool CompatibleTypes = areTypesCompatible(KeyType, ValueType);
+ bool CompatibleTypes = areTypesCompatible(KeyType, ValueType);
+
+ if (PredOp) {
+ if (!hasBuiltinOrder(KeyType))
+ return;
+
+ if (ValueExpr->getType().isVolatileQualified() ||
+ ValueExpr->HasSideEffects(*Result.Context))
+ return;
+
+ // A generic lambda's template parameters are not in scope at the call
+ // site, and its dependent body is not reliably spelled as a
+ // `binaryOperator`.
+ if (Result.Nodes.getNodeAs<LambdaExpr>("Pred")->isGenericLambda())
+ return;
+
+ const QualType PredType =
+ Result.Nodes.getNodeAs<ParmVarDecl>("PredElement")->getType();
+ if (!areTypesCompatible(KeyType, PredType))
+ return;
+
+ // Arithmetic conversions can make the predicate compare as unsigned
+ // while the container method compares as signed.
+ if (KeyType->isSignedIntegerOrEnumerationType() &&
+ ElementExpr->getType()->isUnsignedIntegerOrEnumerationType())
+ return;
+
+ if (!CompatibleTypes) {
+ if (!boundConvertsExactly(ValueExpr, KeyType, *Result.Context))
+ return;
+ CompatibleTypes = true;
+ }
+ }
// Check if the comparison type for the algorithm and the container matches.
if (AlgCall->getNumArgs() == 4 && !Unordered) {
@@ -99,6 +224,13 @@ void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) {
if (Unordered && AlgDecl->getName().contains("bound"))
return;
+ StringRef MethodName = AlgDecl->getName();
+ if (PredOp) {
+ const BinaryOperatorKind Opcode = PredOp->getOpcode();
+ MethodName =
+ Opcode == BO_GT || Opcode == BO_LT ? "upper_bound" : "lower_bound";
+ }
+
const auto *IneffContExpr = Result.Nodes.getNodeAs<Expr>("IneffContExpr");
FixItHint Hint;
@@ -130,7 +262,7 @@ void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) {
CharSourceRange::getTokenRange(IneffContExpr->getSourceRange()), SM,
LangOpts);
const StringRef ParamText = Lexer::getSourceText(
- CharSourceRange::getTokenRange(AlgCall->getArg(2)->getSourceRange()),
+ CharSourceRange::getTokenRange(ValueExprAsWritten->getSourceRange()),
SM, LangOpts);
// There is no source text for an expression that covers only part of a
// macro expansion. Building the replacement from an empty string would
@@ -138,15 +270,16 @@ void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) {
if (!ContainerText.empty() && !ParamText.empty()) {
const std::string ReplacementText =
(llvm::Twine(ContainerText) + (PtrToContainer ? "->" : ".") +
- AlgDecl->getName() + "(" + ParamText + ")")
+ MethodName + "(" + ParamText + ")")
.str();
Hint = FixItHint::CreateReplacement(CallRange, ReplacementText);
}
}
diag(AlgCall->getBeginLoc(),
- "this STL algorithm call should be replaced with a container method")
- << Hint;
+ "this STL algorithm call should be replaced with the container "
+ "method '%0'")
+ << MethodName << Hint;
}
} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h
index e04da47322623..6ee0bf972d5de 100644
--- a/clang-tools-extra/clang-tidy/utils/Matchers.h
+++ b/clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -57,6 +57,12 @@ AST_MATCHER(QualType, isSimpleChar) {
ActualType->isSpecificBuiltinType(BuiltinType::Char_U));
}
+/// Matches a lambda whose call operator matches `InnerMatcher`.
+AST_MATCHER_P(LambdaExpr, hasCallOperator,
+ ast_matchers::internal::Matcher<CXXMethodDecl>, InnerMatcher) {
+ return InnerMatcher.matches(*Node.getCallOperator(), Finder, Builder);
+}
+
AST_MATCHER(Expr, hasUnevaluatedContext) {
if (isa<CXXNoexceptExpr>(Node) || isa<RequiresExpr>(Node))
return true;
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 10d313b52d331..1a60b609fea25 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -161,12 +161,18 @@ infrastructure are described first, followed by tool-specific sections.
different constructor.
- Improved {doc}`performance-inefficient-algorithm
- <clang-tidy/checks/performance/inefficient-algorithm>` check to no longer
- produce a fix with the container or the searched-for value missing, such as
- `.find(43)` or `s.find()`, when either comes from a macro. The value is
- copied as written rather than with its parentheses stripped, and no fix is
- offered when an argument covers only part of a macro expansion, as it then
- has no source text of its own.
+ <clang-tidy/checks/performance/inefficient-algorithm>` check:
+
+ - Added a diagnostic for a `std::find_if` over a `std::set` or
+ `std::multiset` whose predicate compares the element against a fixed bound
+ with a built-in `<`, `<=`, `>` or `>=`. `upper_bound` and `lower_bound`
+ find that element in logarithmic instead of linear time.
+
+ - No longer produces a fix with the container or the searched-for value
+ missing, such as `.find(43)` or `s.find()`, when either comes from a macro.
+ The value is copied as written rather than with its parentheses stripped,
+ and no fix is offered when an argument covers only part of a macro
+ expansion, as it then has no source text of its own.
- Improved {doc}`readability-enum-initial-value
<clang-tidy/checks/readability/enum-initial-value>` check by adding
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-algorithm.md b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-algorithm.md
index d96149601f33b..75cc4853e5420 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-algorithm.md
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-algorithm.md
@@ -26,3 +26,18 @@ auto c = std::count(s.begin(), s.end(), 43);
auto c = s.count(43);
```
+
+In a `std::set` or `std::multiset` that `std::less` orders, finding the first
+element past a fixed bound can use binary search instead of a linear scan:
+
+```cpp
+std::set<int> s;
+auto it = std::find_if(s.begin(), s.end(), [](int v) { return v > 43; });
+
+// becomes
+
+auto it = s.upper_bound(43);
+```
+
+`>` maps to `upper_bound` and `>=` to `lower_bound`. Only built-in comparison
+operators are rewritten.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp
index c4f685f35b13c..a2e243179102f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp
@@ -1,20 +1,32 @@
-// RUN: %check_clang_tidy %s performance-inefficient-algorithm %t
+// RUN: %check_clang_tidy -std=c++11 %s performance-inefficient-algorithm %t
+// RUN: %check_clang_tidy -std=c++14,c++17 %s performance-inefficient-algorithm %t
+// RUN: %check_clang_tidy -std=c++20-or-later %s performance-inefficient-algorithm %t
namespace std {
template <typename T> struct less {
bool operator()(const T &lhs, const T &rhs) { return lhs < rhs; }
};
+template <> struct less<void> {
+ template <typename T, typename U>
+ bool operator()(const T &lhs, const U &rhs) const { return lhs < rhs; }
+};
+
template <typename T> struct greater {
bool operator()(const T &lhs, const T &rhs) { return lhs > rhs; }
};
+template <typename T> struct allocator {};
+
struct iterator_type {};
-template <typename K, typename Cmp = less<K>> struct set {
+template <typename K, typename Cmp = less<K>, typename Alloc = allocator<K>>
+struct set {
typedef iterator_type iterator;
iterator find(const K &k);
unsigned count(const K &k);
+ iterator lower_bound(const K &k);
+ iterator upper_bound(const K &k);
iterator begin();
iterator end();
@@ -41,7 +53,8 @@ template <typename K, typename V> struct unordered_map : map<K, V> {};
template <typename K> struct unordered_multiset : set<K> {};
template <typename K, typename V> struct unordered_multimap : map<K, V> {};
-template <typename K, typename Cmp = less<K>> struct multiset : set<K, Cmp> {};
+template <typename K, typename Cmp = less<K>, typename Alloc = allocator<K>>
+struct multiset : set<K, Cmp, Alloc> {};
template <typename FwIt, typename K>
FwIt find(FwIt, FwIt end, const K &) { return end; }
@@ -78,7 +91,7 @@ template <typename T> void f(const T &t) {
int main() {
std::set<int> s;
auto it = std::find(s.begin(), s.end(), 43);
- // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: this STL algorithm call should be replaced with a container method [performance-inefficient-algorithm]
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: this STL algorithm call should be replaced with the container method 'find' [performance-inefficient-algorithm]
// CHECK-FIXES: auto it = s.find(43);
auto c = count(s.begin(), s.end(), 43);
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: this STL algorithm call should be
@@ -266,3 +279,190 @@ void macroContainer(std::set<int> s, std::set<int> *p) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
// CHECK-FIXES: find(RANGE, PLAIN_VALUE);
}
+
+int getBound();
+
+void findIf(std::set<int> s, std::multiset<int> ms, int i) {
+ auto a = std::find_if(s.begin(), s.end(), [&](int val) { return val > i; });
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: this STL algorithm call should be replaced with the container method 'upper_bound' [performance-inefficient-algorithm]
+ // CHECK-FIXES: auto a = s.upper_bound(i);
+
+ auto b = find_if(s.begin(), s.end(), [&](int val) { return val >= i; });
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: this STL algorithm call should be replaced with the container method 'lower_bound' [performance-inefficient-algorithm]
+ // CHECK-FIXES: auto b = s.lower_bound(i);
+
+ find_if(s.begin(), s.end(), [&](int val) { return i < val; });
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be replaced with the container method 'upper_bound'
+ // CHECK-FIXES: s.upper_bound(i);
+
+ find_if(s.begin(), s.end(), [&](int val) { return i <= val; });
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be replaced with the c...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/218005
More information about the cfe-commits
mailing list