[ADT][NFCI]: Fix iterator category for graph iterators with external storage (#116403)

Set the iterator category for graph iterators to input_iterator_tag when
the visited set is stored externally. In that case we can't provide
multi-pass guarantee, so we should not claim to be a forward iterator.

Fixes: #116400

Author: Maetveis

llvm/include/llvm/ADT/DepthFirstIterator.h

@@ -39,6 +39,7 @@
 #include "llvm/ADT/iterator_range.h"
 #include <iterator>
 #include <optional>
+#include <type_traits>
 #include <utility>
 #include <vector>
 
@@ -84,7 +85,10 @@ template <class GraphT,
           bool ExtStorage = false, class GT = GraphTraits<GraphT>>
 class df_iterator : public df_iterator_storage<SetType, ExtStorage> {
 public:
-  using iterator_category = std::forward_iterator_tag;
+  // When External storage is used we are not multi-pass safe.
+  using iterator_category =
+      std::conditional_t<ExtStorage, std::input_iterator_tag,
+                         std::forward_iterator_tag>;
   using value_type = typename GT::NodeRef;
   using difference_type = std::ptrdiff_t;
   using pointer = value_type *;

llvm/include/llvm/ADT/PostOrderIterator.h

@@ -23,6 +23,7 @@
 #include <iterator>
 #include <optional>
 #include <set>
+#include <type_traits>
 #include <utility>
 
 namespace llvm {
@@ -95,7 +96,10 @@ template <class GraphT,
           bool ExtStorage = false, class GT = GraphTraits<GraphT>>
 class po_iterator : public po_iterator_storage<SetType, ExtStorage> {
 public:
-  using iterator_category = std::forward_iterator_tag;
+  // When External storage is used we are not multi-pass safe.
+  using iterator_category =
+      std::conditional_t<ExtStorage, std::input_iterator_tag,
+                         std::forward_iterator_tag>;
   using value_type = typename GT::NodeRef;
   using difference_type = std::ptrdiff_t;
   using pointer = value_type *;

llvm/unittests/ADT/BreadthFirstIteratorTest.cpp

@@ -10,6 +10,12 @@
 #include "TestGraph.h"
 #include "gtest/gtest.h"
 
+#include <array>
+#include <iterator>
+#include <type_traits>
+
+#include <cstddef>
+
 using namespace llvm;
 
 namespace llvm {
@@ -74,4 +80,29 @@ static_assert(
     std::is_convertible_v<decltype(*std::declval<bf_iterator<Graph<3>>>()),
                           typename bf_iterator<Graph<3>>::reference>);
 
+// bf_iterator should be (at-least) a forward-iterator
+static_assert(std::is_base_of_v<std::forward_iterator_tag,
+                                bf_iterator<Graph<4>>::iterator_category>);
+
+TEST(BreadthFristIteratorTest, MultiPassSafeWithInternalSet) {
+  Graph<4> G;
+  G.AddEdge(0, 1);
+  G.AddEdge(1, 2);
+  G.AddEdge(1, 3);
+
+  std::array<decltype(G)::NodeType *, 4> NodesFirstPass, NodesSecondPass;
+
+  auto B = bf_begin(G), E = bf_end(G);
+
+  std::size_t I = 0;
+  for (auto It = B; It != E; ++It)
+    NodesFirstPass[I++] = *It;
+
+  I = 0;
+  for (auto It = B; It != E; ++It)
+    NodesSecondPass[I++] = *It;
+
+  EXPECT_EQ(NodesFirstPass, NodesSecondPass);
+}
+
 } // end namespace llvm

llvm/unittests/ADT/DepthFirstIteratorTest.cpp

@@ -10,6 +10,12 @@
 #include "TestGraph.h"
 #include "gtest/gtest.h"
 
+#include <array>
+#include <iterator>
+#include <type_traits>
+
+#include <cstddef>
+
 using namespace llvm;
 
 namespace llvm {
@@ -55,4 +61,33 @@ static_assert(
     std::is_convertible_v<decltype(*std::declval<df_iterator<Graph<3>>>()),
                           typename df_iterator<Graph<3>>::reference>);
 
+// df_iterator should be (at-least) a forward-iterator
+static_assert(std::is_base_of_v<std::forward_iterator_tag,
+                                df_iterator<Graph<4>>::iterator_category>);
+
+// df_ext_iterator cannot provide multi-pass guarantee, therefore its only
+// an input-iterator
+static_assert(std::is_same_v<df_ext_iterator<Graph<4>>::iterator_category,
+                             std::input_iterator_tag>);
+
+TEST(DepthFirstIteratorTest, MultiPassSafeWithInternalSet) {
+  Graph<4> G;
+  G.AddEdge(0, 1);
+  G.AddEdge(1, 2);
+  G.AddEdge(1, 3);
+
+  std::array<decltype(G)::NodeType *, 4> NodesFirstPass, NodesSecondPass;
+
+  auto B = df_begin(G), E = df_end(G);
+
+  std::size_t I = 0;
+  for (auto It = B; It != E; ++It)
+    NodesFirstPass[I++] = *It;
+
+  I = 0;
+  for (auto It = B; It != E; ++It)
+    NodesSecondPass[I++] = *It;
+
+  EXPECT_EQ(NodesFirstPass, NodesSecondPass);
+}
 }

llvm/unittests/ADT/PostOrderIteratorTest.cpp

@@ -11,6 +11,12 @@
 #include "gtest/gtest.h"
 #include "TestGraph.h"
 
+#include <array>
+#include <iterator>
+#include <type_traits>
+
+#include <cstddef>
+
 using namespace llvm;
 
 namespace {
@@ -75,4 +81,34 @@ TEST(PostOrderIteratorTest, PostOrderAndReversePostOrderTraverrsal) {
   EXPECT_EQ(1, FromIterator[4]);
   EXPECT_EQ(4, FromIterator[5]);
 }
+
+// po_iterator should be (at-least) a forward-iterator
+static_assert(std::is_base_of_v<std::forward_iterator_tag,
+                                po_iterator<Graph<4>>::iterator_category>);
+
+// po_ext_iterator cannot provide multi-pass guarantee, therefore its only
+// an input-iterator
+static_assert(std::is_same_v<po_ext_iterator<Graph<4>>::iterator_category,
+                             std::input_iterator_tag>);
+
+TEST(PostOrderIteratorTest, MultiPassSafeWithInternalSet) {
+  Graph<4> G;
+  G.AddEdge(0, 1);
+  G.AddEdge(1, 2);
+  G.AddEdge(1, 3);
+
+  std::array<decltype(G)::NodeType *, 4> NodesFirstPass, NodesSecondPass;
+
+  auto B = po_begin(G), E = po_end(G);
+
+  std::size_t I = 0;
+  for (auto It = B; It != E; ++It)
+    NodesFirstPass[I++] = *It;
+
+  I = 0;
+  for (auto It = B; It != E; ++It)
+    NodesSecondPass[I++] = *It;
+
+  EXPECT_EQ(NodesFirstPass, NodesSecondPass);
+}
 }