[NFC][SpecialCaseList] Replace callback with return value #165943
Merged
+45
−50
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
Created using spr 1.3.7
Member
|
@llvm/pr-subscribers-llvm-support Author: Vitaly Buka (vitalybuka) ChangesThis commit introduces Full diff: https://github.com/llvm/llvm-project/pull/165943.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Support/SpecialCaseList.h b/llvm/include/llvm/Support/SpecialCaseList.h
index cb8e568de02e0..5ed7adeaf6c92 100644
--- a/llvm/include/llvm/Support/SpecialCaseList.h
+++ b/llvm/include/llvm/Support/SpecialCaseList.h
@@ -126,15 +126,16 @@ class SpecialCaseList {
SpecialCaseList &operator=(SpecialCaseList const &) = delete;
private:
+ using Match = std::pair<StringRef, unsigned>;
+ static constexpr Match NotMatched = {"", 0};
+
// Lagacy v1 matcher.
class RegexMatcher {
public:
LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber);
LLVM_ABI void preprocess(bool BySize);
- LLVM_ABI void
- match(StringRef Query,
- llvm::function_ref<void(StringRef Rule, unsigned LineNo)> Cb) const;
+ LLVM_ABI Match match(StringRef Query) const;
struct Reg {
Reg(StringRef Name, unsigned LineNo, Regex &&Rg)
@@ -152,9 +153,7 @@ class SpecialCaseList {
LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber);
LLVM_ABI void preprocess(bool BySize);
- LLVM_ABI void
- match(StringRef Query,
- llvm::function_ref<void(StringRef Rule, unsigned LineNo)> Cb) const;
+ LLVM_ABI Match match(StringRef Query) const;
struct Glob {
Glob(StringRef Name, unsigned LineNo, GlobPattern &&Pattern)
@@ -168,11 +167,10 @@ class SpecialCaseList {
RadixTree<iterator_range<StringRef::const_iterator>,
RadixTree<iterator_range<StringRef::const_reverse_iterator>,
- SmallVector<const GlobMatcher::Glob *, 1>>>
+ SmallVector<int, 1>>>
PrefixSuffixToGlob;
- RadixTree<iterator_range<StringRef::const_iterator>,
- SmallVector<const GlobMatcher::Glob *, 1>>
+ RadixTree<iterator_range<StringRef::const_iterator>, SmallVector<int, 1>>
SubstrToGlob;
};
@@ -184,14 +182,10 @@ class SpecialCaseList {
LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber);
LLVM_ABI void preprocess(bool BySize);
- LLVM_ABI void
- match(StringRef Query,
- llvm::function_ref<void(StringRef Rule, unsigned LineNo)> Cb) const;
+ LLVM_ABI Match match(StringRef Query) const;
LLVM_ABI bool matchAny(StringRef Query) const {
- bool R = false;
- match(Query, [&](StringRef, unsigned) { R = true; });
- return R;
+ return match(Query) != NotMatched;
}
std::variant<RegexMatcher, GlobMatcher> M;
diff --git a/llvm/lib/Support/SpecialCaseList.cpp b/llvm/lib/Support/SpecialCaseList.cpp
index 246d90cce3a43..8e6e9f34a73f3 100644
--- a/llvm/lib/Support/SpecialCaseList.cpp
+++ b/llvm/lib/Support/SpecialCaseList.cpp
@@ -15,11 +15,13 @@
#include "llvm/Support/SpecialCaseList.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/LineIterator.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
#include <algorithm>
#include <limits>
#include <memory>
@@ -63,12 +65,12 @@ void SpecialCaseList::RegexMatcher::preprocess(bool BySize) {
}
}
-void SpecialCaseList::RegexMatcher::match(
- StringRef Query,
- llvm::function_ref<void(StringRef Rule, unsigned LineNo)> Cb) const {
+SpecialCaseList::Match
+SpecialCaseList::RegexMatcher::match(StringRef Query) const {
for (const auto &R : reverse(RegExes))
if (R.Rg.match(Query))
- return Cb(R.Name, R.LineNo);
+ return {R.Name, R.LineNo};
+ return NotMatched;
}
Error SpecialCaseList::GlobMatcher::insert(StringRef Pattern,
@@ -90,7 +92,7 @@ void SpecialCaseList::GlobMatcher::preprocess(bool BySize) {
});
}
- for (const auto &G : reverse(Globs)) {
+ for (const auto &[Idx, G] : enumerate(Globs)) {
StringRef Prefix = G.Pattern.prefix();
StringRef Suffix = G.Pattern.suffix();
@@ -102,26 +104,29 @@ void SpecialCaseList::GlobMatcher::preprocess(bool BySize) {
// But only if substring is not empty. Searching this tree is more
// expensive.
auto &V = SubstrToGlob.emplace(Substr).first->second;
- V.emplace_back(&G);
+ V.emplace_back(Idx);
continue;
}
}
auto &SToGlob = PrefixSuffixToGlob.emplace(Prefix).first->second;
auto &V = SToGlob.emplace(reverse(Suffix)).first->second;
- V.emplace_back(&G);
+ V.emplace_back(Idx);
}
}
-void SpecialCaseList::GlobMatcher::match(
- StringRef Query,
- llvm::function_ref<void(StringRef Rule, unsigned LineNo)> Cb) const {
+SpecialCaseList::Match
+SpecialCaseList::GlobMatcher::match(StringRef Query) const {
+ int Best = -1;
if (!PrefixSuffixToGlob.empty()) {
for (const auto &[_, SToGlob] : PrefixSuffixToGlob.find_prefixes(Query)) {
for (const auto &[_, V] : SToGlob.find_prefixes(reverse(Query))) {
- for (const auto *G : V) {
- if (G->Pattern.match(Query)) {
- Cb(G->Name, G->LineNo);
+ for (int Idx : reverse(V)) {
+ if (Best > Idx)
+ break;
+ const GlobMatcher::Glob &G = Globs[Idx];
+ if (G.Pattern.match(Query)) {
+ Best = Idx;
// As soon as we find a match in the vector, we can break for this
// vector, since the globs are already sorted by priority within the
// prefix group. However, we continue searching other prefix groups
@@ -138,9 +143,12 @@ void SpecialCaseList::GlobMatcher::match(
// possibilities. In most cases search will fail on first characters.
for (StringRef Q = Query; !Q.empty(); Q = Q.drop_front()) {
for (const auto &[_, V] : SubstrToGlob.find_prefixes(Q)) {
- for (const auto *G : V) {
- if (G->Pattern.match(Query)) {
- Cb(G->Name, G->LineNo);
+ for (int Idx : reverse(V)) {
+ if (Best > Idx)
+ break;
+ const GlobMatcher::Glob &G = Globs[Idx];
+ if (G.Pattern.match(Query)) {
+ Best = Idx;
// As soon as we find a match in the vector, we can break for this
// vector, since the globs are already sorted by priority within the
// prefix group. However, we continue searching other prefix groups
@@ -151,6 +159,9 @@ void SpecialCaseList::GlobMatcher::match(
}
}
}
+ if (Best < 0)
+ return NotMatched;
+ return {Globs[Best].Name, Globs[Best].LineNo};
}
SpecialCaseList::Matcher::Matcher(bool UseGlobs, bool RemoveDotSlash)
@@ -169,12 +180,11 @@ void SpecialCaseList::Matcher::preprocess(bool BySize) {
return std::visit([&](auto &V) { return V.preprocess(BySize); }, M);
}
-void SpecialCaseList::Matcher::match(
- StringRef Query,
- llvm::function_ref<void(StringRef Rule, unsigned LineNo)> Cb) const {
+SpecialCaseList::Match SpecialCaseList::Matcher::match(StringRef Query) const {
if (RemoveDotSlash)
Query = llvm::sys::path::remove_leading_dotslash(Query);
- return std::visit([&](auto &V) { return V.match(Query, Cb); }, M);
+ return std::visit(
+ [&](auto &V) -> SpecialCaseList::Match { return V.match(Query); }, M);
}
// TODO: Refactor this to return Expected<...>
@@ -371,26 +381,17 @@ LLVM_ABI void SpecialCaseList::Section::preprocess(bool OrderBySize) {
unsigned SpecialCaseList::Section::getLastMatch(StringRef Prefix,
StringRef Query,
StringRef Category) const {
- unsigned LastLine = 0;
- if (const Matcher *M = findMatcher(Prefix, Category)) {
- M->match(Query, [&](StringRef, unsigned LineNo) {
- LastLine = std::max(LastLine, LineNo);
- });
- }
- return LastLine;
+ if (const Matcher *M = findMatcher(Prefix, Category))
+ return M->match(Query).second;
+ return 0;
}
StringRef SpecialCaseList::Section::getLongestMatch(StringRef Prefix,
StringRef Query,
StringRef Category) const {
- StringRef LongestRule;
- if (const Matcher *M = findMatcher(Prefix, Category)) {
- M->match(Query, [&](StringRef Rule, unsigned) {
- if (LongestRule.size() < Rule.size())
- LongestRule = Rule;
- });
- }
- return LongestRule;
+ if (const Matcher *M = findMatcher(Prefix, Category))
+ return M->match(Query).first;
+ return {};
}
} // namespace llvm
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SpecialCaseList matching API by replacing callback-based pattern matching with a return-value-based approach. The change simplifies the API by returning a Match pair (pattern name and line number) directly instead of invoking callbacks for each match.
Key changes:
- Refactored
RegexMatcher::match(),GlobMatcher::match(), andMatcher::match()to returnMatch(pair of StringRef and unsigned) instead of using callbacks - Changed internal data structures in
GlobMatcherfrom storing pointers toGlobobjects to storing indices into theGlobsvector - Simplified
getLastMatch()andgetLongestMatch()methods to directly use the returned match result
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| llvm/include/llvm/Support/SpecialCaseList.h | Updated matcher method signatures to return Match type and changed internal storage from glob pointers to indices |
| llvm/lib/Support/SpecialCaseList.cpp | Implemented new matching logic with index-based tracking and added unused header includes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
vitalybuka
added a commit
to vitalybuka/llvm-project
that referenced
this pull request
Nov 10, 2025
This commit introduces `SpecialCaseList::Match`, a small struct to hold the matched rule and its line number. This simplifies the `match` methods by allowing them to return a single value instead of using a callback. Pull Request: llvm#165943
vitalybuka
added a commit
to vitalybuka/llvm-project
that referenced
this pull request
Nov 10, 2025
This commit introduces `SpecialCaseList::Match`, a small struct to hold the matched rule and its line number. This simplifies the `match` methods by allowing them to return a single value instead of using a callback. Pull Request: llvm#165943
qinkunbao
approved these changes
Nov 10, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vitalybuka
deleted the
users/vitalybuka/spr/nfcspecialcaselist-replace-callback-with-return-value
branch
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/32833 Here is the relevant piece of the build log for the reference |
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.
This commit introduces
SpecialCaseList::Match, a small struct to holdthe matched rule and its line number. This simplifies the
matchmethods by allowing them to return a single value instead of using a
callback.