Skip to content

Commit 2a49ebf

Browse files
[ADT] Simplify StringMapIterBase (NFC) (#156392)
In open-adressing hash tables, begin() needs to advance to the first valid element. We don't need to do the same for any other operations like end(), find(), and try_emplace(). The problem is that the constructor of StringMapIterBase says: bool NoAdvance = false This increases the burden on the callers because most places need to pass true for NoAdvance, defeating the benefit of the default parameter. This patch fixes the problem by changing the name and default to: bool Advance = false and adjusting callers. Again, begin() is the only caller that specifies this parameter. This patch fixes a "latent bug" where try_emplace() was requesting advancing even on a successful insertion. I say "latent" because the request is a no-op on success.
1 parent 5e91314 commit 2a49ebf

File tree

1 file changed

+11
-15
lines changed

1 file changed

+11
-15
lines changed

llvm/include/llvm/ADT/StringMap.h

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -220,14 +220,12 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
220220
using const_iterator = StringMapIterBase<ValueTy, true>;
221221
using iterator = StringMapIterBase<ValueTy, false>;
222222

223-
iterator begin() { return iterator(TheTable, NumBuckets == 0); }
224-
iterator end() { return iterator(TheTable + NumBuckets, true); }
223+
iterator begin() { return iterator(TheTable, NumBuckets != 0); }
224+
iterator end() { return iterator(TheTable + NumBuckets); }
225225
const_iterator begin() const {
226-
return const_iterator(TheTable, NumBuckets == 0);
227-
}
228-
const_iterator end() const {
229-
return const_iterator(TheTable + NumBuckets, true);
226+
return const_iterator(TheTable, NumBuckets != 0);
230227
}
228+
const_iterator end() const { return const_iterator(TheTable + NumBuckets); }
231229

232230
iterator_range<StringMapKeyIterator<ValueTy>> keys() const {
233231
return make_range(StringMapKeyIterator<ValueTy>(begin()),
@@ -240,7 +238,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
240238
int Bucket = FindKey(Key, FullHashValue);
241239
if (Bucket == -1)
242240
return end();
243-
return iterator(TheTable + Bucket, true);
241+
return iterator(TheTable + Bucket);
244242
}
245243

246244
const_iterator find(StringRef Key) const { return find(Key, hash(Key)); }
@@ -249,7 +247,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
249247
int Bucket = FindKey(Key, FullHashValue);
250248
if (Bucket == -1)
251249
return end();
252-
return const_iterator(TheTable + Bucket, true);
250+
return const_iterator(TheTable + Bucket);
253251
}
254252

255253
/// lookup - Return the entry for the specified key, or a default
@@ -380,8 +378,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
380378
unsigned BucketNo = LookupBucketFor(Key, FullHashValue);
381379
StringMapEntryBase *&Bucket = TheTable[BucketNo];
382380
if (Bucket && Bucket != getTombstoneVal())
383-
return {iterator(TheTable + BucketNo, false),
384-
false}; // Already exists in map.
381+
return {iterator(TheTable + BucketNo), false}; // Already exists in map.
385382

386383
if (Bucket == getTombstoneVal())
387384
--NumTombstones;
@@ -391,7 +388,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
391388
assert(NumItems + NumTombstones <= NumBuckets);
392389

393390
BucketNo = RehashTable(BucketNo);
394-
return {iterator(TheTable + BucketNo, false), true};
391+
return {iterator(TheTable + BucketNo), true};
395392
}
396393

397394
// clear - Empties out the StringMap
@@ -444,10 +441,9 @@ template <typename ValueTy, bool IsConst> class StringMapIterBase {
444441

445442
StringMapIterBase() = default;
446443

447-
explicit StringMapIterBase(StringMapEntryBase **Bucket,
448-
bool NoAdvance = false)
444+
explicit StringMapIterBase(StringMapEntryBase **Bucket, bool Advance = false)
449445
: Ptr(Bucket) {
450-
if (!NoAdvance)
446+
if (Advance)
451447
AdvancePastEmptyBuckets();
452448
}
453449

@@ -469,7 +465,7 @@ template <typename ValueTy, bool IsConst> class StringMapIterBase {
469465
template <bool ToConst,
470466
typename = typename std::enable_if<!IsConst && ToConst>::type>
471467
operator StringMapIterBase<ValueTy, ToConst>() const {
472-
return StringMapIterBase<ValueTy, ToConst>(Ptr, true);
468+
return StringMapIterBase<ValueTy, ToConst>(Ptr);
473469
}
474470

475471
friend bool operator==(const StringMapIterBase &LHS,

0 commit comments

Comments
 (0)