[llvm] c88a7a7 - [llvm][ADT] Fix invalid `reference` type of depth-first, breadth-first and post order iterators

[Markus Böck via llvm-commits]

Author: Markus Böck
Date: 2023-05-23T19:29:58+02:00
New Revision: c88a7a7b8ebf5b08fbbac2d22c07810774af231b

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

LOG: [llvm][ADT] Fix invalid `reference` type of depth-first, breadth-first and post order iterators

C++s iterator concept requires operator* to return the same type as is specified by the iterators reference type. This functionality is especially important for older generic code that did not yet make use of auto.
An example from within LLVM is iterator_adaptor_base which uses the reference type of the iterator it is wrapping as its return type for operator* (this class is used as base for a lot of other functionality like filter iterators and so on).
Using any of the graph traversal iterators listed above with it would previously fail to compile due to reference being non-const while operator* returned a const reference.

This patch fixes that by correctly specifying reference and using it as the return type of operator* explicitly to prevent further issues in the future.

Differential Revision: https://reviews.llvm.org/D151198

Added: 
    

Modified: 
    llvm/include/llvm/ADT/BreadthFirstIterator.h
    llvm/include/llvm/ADT/DepthFirstIterator.h
    llvm/include/llvm/ADT/PostOrderIterator.h
    llvm/unittests/ADT/BreadthFirstIteratorTest.cpp
    llvm/unittests/ADT/DepthFirstIteratorTest.cpp
    llvm/unittests/ADT/PostOrderIteratorTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ADT/BreadthFirstIterator.h b/llvm/include/llvm/ADT/BreadthFirstIterator.h
index c1a236bf56923..29e96693c4d1a 100644
--- a/llvm/include/llvm/ADT/BreadthFirstIterator.h
+++ b/llvm/include/llvm/ADT/BreadthFirstIterator.h
@@ -50,7 +50,7 @@ class bf_iterator : public bf_iterator_storage<SetType> {
   using value_type = typename GT::NodeRef;
   using 
diff erence_type = std::ptr
diff _t;
   using pointer = value_type *;
-  using reference = value_type &;
+  using reference = const value_type &;
 
 private:
   using NodeRef = typename GT::NodeRef;
@@ -123,7 +123,7 @@ class bf_iterator : public bf_iterator_storage<SetType> {
 
   bool operator!=(const bf_iterator &RHS) const { return !(*this == RHS); }
 
-  const NodeRef &operator*() const { return VisitQueue.front()->first; }
+  reference operator*() const { return VisitQueue.front()->first; }
 
   // This is a nonstandard operator-> that dereferences the pointer an extra
   // time so that you can actually call methods on the node, because the

diff  --git a/llvm/include/llvm/ADT/DepthFirstIterator.h b/llvm/include/llvm/ADT/DepthFirstIterator.h
index 29ea2d541d9f3..71053c2d0d8a8 100644
--- a/llvm/include/llvm/ADT/DepthFirstIterator.h
+++ b/llvm/include/llvm/ADT/DepthFirstIterator.h
@@ -88,7 +88,7 @@ class df_iterator : public df_iterator_storage<SetType, ExtStorage> {
   using value_type = typename GT::NodeRef;
   using 
diff erence_type = std::ptr
diff _t;
   using pointer = value_type *;
-  using reference = value_type &;
+  using reference = const value_type &;
 
 private:
   using NodeRef = typename GT::NodeRef;
@@ -165,7 +165,7 @@ class df_iterator : public df_iterator_storage<SetType, ExtStorage> {
   }
   bool operator!=(const df_iterator &x) const { return !(*this == x); }
 
-  const NodeRef &operator*() const { return VisitStack.back().first; }
+  reference operator*() const { return VisitStack.back().first; }
 
   // This is a nonstandard operator-> that dereferences the pointer an extra
   // time... so that you can actually call methods ON the Node, because

diff  --git a/llvm/include/llvm/ADT/PostOrderIterator.h b/llvm/include/llvm/ADT/PostOrderIterator.h
index d13a53f382901..33d3330a40bd3 100644
--- a/llvm/include/llvm/ADT/PostOrderIterator.h
+++ b/llvm/include/llvm/ADT/PostOrderIterator.h
@@ -100,7 +100,7 @@ class po_iterator : public po_iterator_storage<SetType, ExtStorage> {
   using value_type = typename GT::NodeRef;
   using 
diff erence_type = std::ptr
diff _t;
   using pointer = value_type *;
-  using reference = value_type &;
+  using reference = const value_type &;
 
 private:
   using NodeRef = typename GT::NodeRef;
@@ -161,7 +161,7 @@ class po_iterator : public po_iterator_storage<SetType, ExtStorage> {
   }
   bool operator!=(const po_iterator &x) const { return !(*this == x); }
 
-  const NodeRef &operator*() const { return std::get<0>(VisitStack.back()); }
+  reference operator*() const { return std::get<0>(VisitStack.back()); }
 
   // This is a nonstandard operator-> that dereferences the pointer an extra
   // time... so that you can actually call methods ON the BasicBlock, because

diff  --git a/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp b/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp
index 5d034ad41fc7c..5428c644d9ab2 100644
--- a/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp
+++ b/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp
@@ -70,4 +70,8 @@ TEST(BreadthFristIteratorTest, Cycle) {
   EXPECT_EQ(It, End);
 }
 
+static_assert(
+    std::is_convertible_v<decltype(*std::declval<bf_iterator<Graph<3>>>()),
+                          typename bf_iterator<Graph<3>>::reference>);
+
 } // end namespace llvm

diff  --git a/llvm/unittests/ADT/DepthFirstIteratorTest.cpp b/llvm/unittests/ADT/DepthFirstIteratorTest.cpp
index 0e3e0e66d71e0..007b6839f4d05 100644
--- a/llvm/unittests/ADT/DepthFirstIteratorTest.cpp
+++ b/llvm/unittests/ADT/DepthFirstIteratorTest.cpp
@@ -50,4 +50,9 @@ TEST(DepthFirstIteratorTest, ActuallyUpdateIterator) {
 
   EXPECT_EQ(3, S.InsertVisited);
 }
+
+static_assert(
+    std::is_convertible_v<decltype(*std::declval<df_iterator<Graph<3>>>()),
+                          typename df_iterator<Graph<3>>::reference>);
+
 }

diff  --git a/llvm/unittests/ADT/PostOrderIteratorTest.cpp b/llvm/unittests/ADT/PostOrderIteratorTest.cpp
index 528d4bcc99833..17c84229cbc4d 100644
--- a/llvm/unittests/ADT/PostOrderIteratorTest.cpp
+++ b/llvm/unittests/ADT/PostOrderIteratorTest.cpp
@@ -36,6 +36,10 @@ TEST(PostOrderIteratorTest, Compiles) {
   PIExt.insertEdge(std::optional<BasicBlock *>(), NullBB);
 }
 
+static_assert(
+    std::is_convertible_v<decltype(*std::declval<po_iterator<Graph<3>>>()),
+                          typename po_iterator<Graph<3>>::reference>);
+
 // Test post-order and reverse post-order traversals for simple graph type.
 TEST(PostOrderIteratorTest, PostOrderAndReversePostOrderTraverrsal) {
   Graph<6> G;