[llvm] 887efa5 - Fix DenseMap iterator asserts when shouldReverseIterate==true

[Chris Bieneman via llvm-commits]

Author: Chris Bieneman
Date: 2020-04-20T19:31:32-05:00
New Revision: 887efa51c1e0e43ca684ed78b92dbc3a0720881b

URL: https://github.com/llvm/llvm-project/commit/887efa51c1e0e43ca684ed78b92dbc3a0720881b
DIFF: https://github.com/llvm/llvm-project/commit/887efa51c1e0e43ca684ed78b92dbc3a0720881b.diff

LOG: Fix DenseMap iterator asserts when shouldReverseIterate==true

This patch gets the asserts working correctly when LLVM_REVERSE_ITERATION=On by fixing the iterators returned by the DenseMap::find* methods so that they return well-formed iterators that work with reverse iteration, and satisfy the assertions.

Added: 
    

Modified: 
    llvm/include/llvm/ADT/DenseMap.h

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index f453ce539e2b..ff9dab15291f 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -149,13 +149,19 @@ class DenseMapBase : public DebugEpochBase {
   iterator find(const_arg_type_t<KeyT> Val) {
     BucketT *TheBucket;
     if (LookupBucketFor(Val, TheBucket))
-      return makeIterator(TheBucket, getBucketsEnd(), *this, true);
+      return makeIterator(TheBucket,
+                          shouldReverseIterate<KeyT>() ? getBuckets()
+                                                       : getBucketsEnd(),
+                          *this, true);
     return end();
   }
   const_iterator find(const_arg_type_t<KeyT> Val) const {
     const BucketT *TheBucket;
     if (LookupBucketFor(Val, TheBucket))
-      return makeConstIterator(TheBucket, getBucketsEnd(), *this, true);
+      return makeConstIterator(TheBucket,
+                               shouldReverseIterate<KeyT>() ? getBuckets()
+                                                            : getBucketsEnd(),
+                               *this, true);
     return end();
   }
 
@@ -168,14 +174,20 @@ class DenseMapBase : public DebugEpochBase {
   iterator find_as(const LookupKeyT &Val) {
     BucketT *TheBucket;
     if (LookupBucketFor(Val, TheBucket))
-      return makeIterator(TheBucket, getBucketsEnd(), *this, true);
+      return makeIterator(TheBucket,
+                          shouldReverseIterate<KeyT>() ? getBuckets()
+                                                       : getBucketsEnd(),
+                          *this, true);
     return end();
   }
   template<class LookupKeyT>
   const_iterator find_as(const LookupKeyT &Val) const {
     const BucketT *TheBucket;
     if (LookupBucketFor(Val, TheBucket))
-      return makeConstIterator(TheBucket, getBucketsEnd(), *this, true);
+      return makeConstIterator(TheBucket,
+                               shouldReverseIterate<KeyT>() ? getBuckets()
+                                                            : getBucketsEnd(),
+                               *this, true);
     return end();
   }
 
@@ -208,17 +220,17 @@ class DenseMapBase : public DebugEpochBase {
   template <typename... Ts>
   std::pair<iterator, bool> try_emplace(KeyT &&Key, Ts &&... Args) {
     BucketT *TheBucket;
+    BucketT *EndBucket =
+        shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd();
     if (LookupBucketFor(Key, TheBucket))
-      return std::make_pair(
-               makeIterator(TheBucket, getBucketsEnd(), *this, true),
-               false); // Already in map.
+      return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
+                            false); // Already in map.
 
     // Otherwise, insert the new element.
     TheBucket =
         InsertIntoBucket(TheBucket, std::move(Key), std::forward<Ts>(Args)...);
-    return std::make_pair(
-             makeIterator(TheBucket, getBucketsEnd(), *this, true),
-             true);
+    return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
+                          true);
   }
 
   // Inserts key,value pair into the map if the key isn't already in the map.
@@ -227,16 +239,16 @@ class DenseMapBase : public DebugEpochBase {
   template <typename... Ts>
   std::pair<iterator, bool> try_emplace(const KeyT &Key, Ts &&... Args) {
     BucketT *TheBucket;
+    BucketT *EndBucket =
+        shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd();
     if (LookupBucketFor(Key, TheBucket))
-      return std::make_pair(
-               makeIterator(TheBucket, getBucketsEnd(), *this, true),
-               false); // Already in map.
+      return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
+                            false); // Already in map.
 
     // Otherwise, insert the new element.
     TheBucket = InsertIntoBucket(TheBucket, Key, std::forward<Ts>(Args)...);
-    return std::make_pair(
-             makeIterator(TheBucket, getBucketsEnd(), *this, true),
-             true);
+    return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
+                          true);
   }
 
   /// Alternate version of insert() which allows a 
diff erent, and possibly
@@ -248,17 +260,17 @@ class DenseMapBase : public DebugEpochBase {
   std::pair<iterator, bool> insert_as(std::pair<KeyT, ValueT> &&KV,
                                       const LookupKeyT &Val) {
     BucketT *TheBucket;
+    BucketT *EndBucket =
+        shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd();
     if (LookupBucketFor(Val, TheBucket))
-      return std::make_pair(
-               makeIterator(TheBucket, getBucketsEnd(), *this, true),
-               false); // Already in map.
+      return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
+                            false); // Already in map.
 
     // Otherwise, insert the new element.
     TheBucket = InsertIntoBucketWithLookup(TheBucket, std::move(KV.first),
                                            std::move(KV.second), Val);
-    return std::make_pair(
-             makeIterator(TheBucket, getBucketsEnd(), *this, true),
-             true);
+    return std::make_pair(makeIterator(TheBucket, EndBucket, *this, true),
+                          true);
   }
 
   /// insert - Range insertion of pairs.
@@ -1200,20 +1212,16 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
 
   reference operator*() const {
     assert(isHandleInSync() && "invalid iterator access!");
-    if (shouldReverseIterate<KeyT>()) {
-      assert(Ptr != &End[-1] && "dereferencing end() iterator");
-      return Ptr[-1];
-    }
     assert(Ptr != End && "dereferencing end() iterator");
+    if (shouldReverseIterate<KeyT>())
+      return Ptr[-1];
     return *Ptr;
   }
   pointer operator->() const {
     assert(isHandleInSync() && "invalid iterator access!");
-    if (shouldReverseIterate<KeyT>()) {
-      assert(Ptr != &End[-1] && "dereferencing end() iterator");
-      return &(Ptr[-1]);
-    }
     assert(Ptr != End && "dereferencing end() iterator");
+    if (shouldReverseIterate<KeyT>())
+      return &(Ptr[-1]);
     return Ptr;
   }
 
@@ -1234,13 +1242,12 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
 
   inline DenseMapIterator& operator++() {  // Preincrement
     assert(isHandleInSync() && "invalid iterator access!");
+    assert(Ptr != End && "incrementing end() iterator");
     if (shouldReverseIterate<KeyT>()) {
-      assert(Ptr != &End[-1] && "dereferencing end() iterator");
       --Ptr;
       RetreatPastEmptyBuckets();
       return *this;
     }
-    assert(Ptr != End && "incrementing end() iterator");
     ++Ptr;
     AdvancePastEmptyBuckets();
     return *this;