[llvm] [ADT] Simplify StringMapIterBase (NFC) (PR #156392)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

<details>
<summary>Changes</summary>

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.


---
Full diff: https://github.com/llvm/llvm-project/pull/156392.diff


1 Files Affected:

- (modified) llvm/include/llvm/ADT/StringMap.h (+11-15) 


``````````diff
diff --git a/llvm/include/llvm/ADT/StringMap.h b/llvm/include/llvm/ADT/StringMap.h
index ac66444968f8e..2c146fbf08df1 100644
--- a/llvm/include/llvm/ADT/StringMap.h
+++ b/llvm/include/llvm/ADT/StringMap.h
@@ -220,14 +220,12 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
   using const_iterator = StringMapIterBase<ValueTy, true>;
   using iterator = StringMapIterBase<ValueTy, false>;
 
-  iterator begin() { return iterator(TheTable, NumBuckets == 0); }
-  iterator end() { return iterator(TheTable + NumBuckets, true); }
+  iterator begin() { return iterator(TheTable, NumBuckets != 0); }
+  iterator end() { return iterator(TheTable + NumBuckets); }
   const_iterator begin() const {
-    return const_iterator(TheTable, NumBuckets == 0);
-  }
-  const_iterator end() const {
-    return const_iterator(TheTable + NumBuckets, true);
+    return const_iterator(TheTable, NumBuckets != 0);
   }
+  const_iterator end() const { return const_iterator(TheTable + NumBuckets); }
 
   iterator_range<StringMapKeyIterator<ValueTy>> keys() const {
     return make_range(StringMapKeyIterator<ValueTy>(begin()),
@@ -240,7 +238,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
     int Bucket = FindKey(Key, FullHashValue);
     if (Bucket == -1)
       return end();
-    return iterator(TheTable + Bucket, true);
+    return iterator(TheTable + Bucket);
   }
 
   const_iterator find(StringRef Key) const { return find(Key, hash(Key)); }
@@ -249,7 +247,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
     int Bucket = FindKey(Key, FullHashValue);
     if (Bucket == -1)
       return end();
-    return const_iterator(TheTable + Bucket, true);
+    return const_iterator(TheTable + Bucket);
   }
 
   /// lookup - Return the entry for the specified key, or a default
@@ -380,8 +378,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
     unsigned BucketNo = LookupBucketFor(Key, FullHashValue);
     StringMapEntryBase *&Bucket = TheTable[BucketNo];
     if (Bucket && Bucket != getTombstoneVal())
-      return {iterator(TheTable + BucketNo, false),
-              false}; // Already exists in map.
+      return {iterator(TheTable + BucketNo), false}; // Already exists in map.
 
     if (Bucket == getTombstoneVal())
       --NumTombstones;
@@ -391,7 +388,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
     assert(NumItems + NumTombstones <= NumBuckets);
 
     BucketNo = RehashTable(BucketNo);
-    return {iterator(TheTable + BucketNo, false), true};
+    return {iterator(TheTable + BucketNo), true};
   }
 
   // clear - Empties out the StringMap
@@ -444,10 +441,9 @@ template <typename ValueTy, bool IsConst> class StringMapIterBase {
 
   StringMapIterBase() = default;
 
-  explicit StringMapIterBase(StringMapEntryBase **Bucket,
-                             bool NoAdvance = false)
+  explicit StringMapIterBase(StringMapEntryBase **Bucket, bool Advance = false)
       : Ptr(Bucket) {
-    if (!NoAdvance)
+    if (Advance)
       AdvancePastEmptyBuckets();
   }
 
@@ -469,7 +465,7 @@ template <typename ValueTy, bool IsConst> class StringMapIterBase {
   template <bool ToConst,
             typename = typename std::enable_if<!IsConst && ToConst>::type>
   operator StringMapIterBase<ValueTy, ToConst>() const {
-    return StringMapIterBase<ValueTy, ToConst>(Ptr, true);
+    return StringMapIterBase<ValueTy, ToConst>(Ptr);
   }
 
   friend bool operator==(const StringMapIterBase &LHS,

``````````

</details>


https://github.com/llvm/llvm-project/pull/156392