[llvm] [ADT] Overhaul the DenseMapIterator creation logic (NFC) (PR #156221)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Aug 30 20:35:16 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-adt
Author: Kazu Hirata (kazutakahirata)
<details>
<summary>Changes</summary>
Without this patch, it's overly complicated to create iterators in
DenseMap.
- We must specify whether to advance a newly created iterator, which
is needed in begin().
- We call shouldReverseIterate outside and inside DenseMapIterator.
This patch cleans up all this by creating factory methods:
- DenseMapIterator::makeBegin
- DenseMapIterator::makeEnd
- DenseMapIterator::makeIterator
With these:
- makeBegin knows that we need to advance the iterator to the first
valid bucket.
- Callers outside DenseMapIterator do not reference
shouldReverseIterate at all.
Now, it's a lot simpler to call helper functions
DenseMapBase::{makeIterator,makeConstIterator}. We just have to pass
the Bucket pointer:
makeIterator(Bucket);
and they take care of the rest, including passing *this as Epoch.
---
Full diff: https://github.com/llvm/llvm-project/pull/156221.diff
1 Files Affected:
- (modified) llvm/include/llvm/ADT/DenseMap.h (+49-57)
``````````diff
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 4a2da7f630663..0650dc2044ca9 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -76,26 +76,14 @@ class DenseMapBase : public DebugEpochBase {
DenseMapIterator<KeyT, ValueT, KeyInfoT, BucketT, true>;
inline iterator begin() {
- // When the map is empty, avoid the overhead of advancing/retreating past
- // empty buckets.
- if (empty())
- return end();
- if (shouldReverseIterate<KeyT>())
- return makeIterator(getBucketsEnd() - 1, getBuckets(), *this);
- return makeIterator(getBuckets(), getBucketsEnd(), *this);
- }
- inline iterator end() {
- return makeIterator(getBucketsEnd(), getBucketsEnd(), *this, true);
+ return iterator::makeBegin(buckets(), empty(), *this);
}
+ inline iterator end() { return iterator::makeEnd(buckets(), *this); }
inline const_iterator begin() const {
- if (empty())
- return end();
- if (shouldReverseIterate<KeyT>())
- return makeConstIterator(getBucketsEnd() - 1, getBuckets(), *this);
- return makeConstIterator(getBuckets(), getBucketsEnd(), *this);
+ return const_iterator::makeBegin(buckets(), empty(), *this);
}
inline const_iterator end() const {
- return makeConstIterator(getBucketsEnd(), getBucketsEnd(), *this, true);
+ return const_iterator::makeEnd(buckets(), *this);
}
// Return an iterator to iterate over keys in the map.
@@ -184,17 +172,13 @@ class DenseMapBase : public DebugEpochBase {
/// type used.
template <class LookupKeyT> iterator find_as(const LookupKeyT &Val) {
if (BucketT *Bucket = doFind(Val))
- return makeIterator(
- Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
- *this, true);
+ return makeIterator(Bucket);
return end();
}
template <class LookupKeyT>
const_iterator find_as(const LookupKeyT &Val) const {
if (const BucketT *Bucket = doFind(Val))
- return makeConstIterator(
- Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
- *this, true);
+ return makeConstIterator(Bucket);
return end();
}
@@ -264,13 +248,13 @@ class DenseMapBase : public DebugEpochBase {
const LookupKeyT &Val) {
BucketT *TheBucket;
if (LookupBucketFor(Val, TheBucket))
- return {makeInsertIterator(TheBucket), false}; // Already in map.
+ return {makeIterator(TheBucket), false}; // Already in map.
// Otherwise, insert the new element.
TheBucket = findBucketForInsertion(Val, TheBucket);
TheBucket->getFirst() = std::move(KV.first);
::new (&TheBucket->getSecond()) ValueT(std::move(KV.second));
- return {makeInsertIterator(TheBucket), true};
+ return {makeIterator(TheBucket), true};
}
/// insert - Range insertion of pairs.
@@ -482,33 +466,15 @@ class DenseMapBase : public DebugEpochBase {
std::pair<iterator, bool> try_emplace_impl(KeyArgT &&Key, Ts &&...Args) {
auto [Bucket, Inserted] = lookupOrInsertIntoBucket(
std::forward<KeyArgT>(Key), std::forward<Ts>(Args)...);
- return {makeInsertIterator(Bucket), Inserted};
+ return {makeIterator(Bucket), Inserted};
}
- iterator makeIterator(BucketT *P, BucketT *E, DebugEpochBase &Epoch,
- bool NoAdvance = false) {
- if (shouldReverseIterate<KeyT>()) {
- BucketT *B = P == getBucketsEnd() ? getBuckets() : P + 1;
- return iterator(B, E, Epoch, NoAdvance);
- }
- return iterator(P, E, Epoch, NoAdvance);
+ iterator makeIterator(BucketT *TheBucket) {
+ return iterator::makeIterator(TheBucket, buckets(), *this);
}
- const_iterator makeConstIterator(const BucketT *P, const BucketT *E,
- const DebugEpochBase &Epoch,
- const bool NoAdvance = false) const {
- if (shouldReverseIterate<KeyT>()) {
- const BucketT *B = P == getBucketsEnd() ? getBuckets() : P + 1;
- return const_iterator(B, E, Epoch, NoAdvance);
- }
- return const_iterator(P, E, Epoch, NoAdvance);
- }
-
- iterator makeInsertIterator(BucketT *TheBucket) {
- return makeIterator(TheBucket,
- shouldReverseIterate<KeyT>() ? getBuckets()
- : getBucketsEnd(),
- *this, true);
+ const_iterator makeConstIterator(const BucketT *TheBucket) const {
+ return const_iterator::makeIterator(TheBucket, buckets(), *this);
}
unsigned getNumEntries() const {
@@ -555,6 +521,10 @@ class DenseMapBase : public DebugEpochBase {
return llvm::make_range(getBuckets(), getBucketsEnd());
}
+ iterator_range<const BucketT *> buckets() const {
+ return llvm::make_range(getBuckets(), getBucketsEnd());
+ }
+
void grow(unsigned AtLeast) { static_cast<DerivedT *>(this)->grow(AtLeast); }
void shrink_and_clear() { static_cast<DerivedT *>(this)->shrink_and_clear(); }
@@ -1217,21 +1187,43 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
pointer Ptr = nullptr;
pointer End = nullptr;
-public:
- DenseMapIterator() = default;
-
- DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch,
- bool NoAdvance = false)
+ DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch)
: DebugEpochBase::HandleBase(&Epoch), Ptr(Pos), End(E) {
assert(isHandleInSync() && "invalid construction!");
+ }
- if (NoAdvance)
- return;
+public:
+ DenseMapIterator() = default;
+
+ static DenseMapIterator makeBegin(iterator_range<pointer> Buckets,
+ bool IsEmpty, const DebugEpochBase &Epoch) {
+ // When the map is empty, avoid the overhead of advancing/retreating past
+ // empty buckets.
+ if (IsEmpty)
+ return makeEnd(Buckets, Epoch);
if (shouldReverseIterate<KeyT>()) {
- RetreatPastEmptyBuckets();
- return;
+ DenseMapIterator Iter(Buckets.end(), Buckets.begin(), Epoch);
+ Iter.RetreatPastEmptyBuckets();
+ return Iter;
}
- AdvancePastEmptyBuckets();
+ DenseMapIterator Iter(Buckets.begin(), Buckets.end(), Epoch);
+ Iter.AdvancePastEmptyBuckets();
+ return Iter;
+ }
+
+ static DenseMapIterator makeEnd(iterator_range<pointer> Buckets,
+ const DebugEpochBase &Epoch) {
+ if (shouldReverseIterate<KeyT>())
+ return DenseMapIterator(Buckets.begin(), Buckets.begin(), Epoch);
+ return DenseMapIterator(Buckets.end(), Buckets.end(), Epoch);
+ }
+
+ static DenseMapIterator makeIterator(pointer P,
+ iterator_range<pointer> Buckets,
+ const DebugEpochBase &Epoch) {
+ if (shouldReverseIterate<KeyT>())
+ return DenseMapIterator(P + 1, Buckets.begin(), Epoch);
+ return DenseMapIterator(P, Buckets.end(), Epoch);
}
// Converting ctor from non-const iterators to const iterators. SFINAE'd out
``````````
</details>
https://github.com/llvm/llvm-project/pull/156221
More information about the llvm-commits
mailing list