[llvm] [ADT][NFCI]: Fix iterator category for graph iterators with external … (PR #116403)
Mészáros Gergely via llvm-commits
llvm-commits at lists.llvm.org
Sat Nov 16 02:36:32 PST 2024
https://github.com/Maetveis updated https://github.com/llvm/llvm-project/pull/116403
>From 468b66a56a948e1427c99d0c8e699fff89f1b1fd Mon Sep 17 00:00:00 2001
From: Gergely Meszaros <gergely.meszaros at intel.com>
Date: Fri, 15 Nov 2024 07:45:23 -0800
Subject: [PATCH 1/2] [ADT][NFCI]: Fix iterator category for graph iterators
with external storage
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
---
llvm/include/llvm/ADT/DepthFirstIterator.h | 6 +++++-
llvm/include/llvm/ADT/PostOrderIterator.h | 6 +++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/llvm/include/llvm/ADT/DepthFirstIterator.h b/llvm/include/llvm/ADT/DepthFirstIterator.h
index 71053c2d0d8a8e..4ced758343806e 100644
--- a/llvm/include/llvm/ADT/DepthFirstIterator.h
+++ b/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 *;
diff --git a/llvm/include/llvm/ADT/PostOrderIterator.h b/llvm/include/llvm/ADT/PostOrderIterator.h
index edf0401a751708..1cbd3c170052c7 100644
--- a/llvm/include/llvm/ADT/PostOrderIterator.h
+++ b/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 *;
>From b59161510af1f89ea00ec6a29c110abd70b61bc1 Mon Sep 17 00:00:00 2001
From: Gergely Meszaros <gergely.meszaros at intel.com>
Date: Sat, 16 Nov 2024 09:59:11 +0000
Subject: [PATCH 2/2] [ADT][Test]: multi-Pass safety, iterator_category for
graph iterators
Check that using the same iterators for multiple passes produces the
same result, as required by the C++ standard for forward iterators.
Add a static_asserts for the iterator_category of Graph iterators
(BreadthFirstIterator, DepthFirstIterator, PostOrderIterator).
---
.../ADT/BreadthFirstIteratorTest.cpp | 31 ++++++++++++++++
llvm/unittests/ADT/DepthFirstIteratorTest.cpp | 35 ++++++++++++++++++
llvm/unittests/ADT/PostOrderIteratorTest.cpp | 36 +++++++++++++++++++
3 files changed, 102 insertions(+)
diff --git a/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp b/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp
index 5428c644d9ab2a..0cd7fd3f706f04 100644
--- a/llvm/unittests/ADT/BreadthFirstIteratorTest.cpp
+++ b/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
diff --git a/llvm/unittests/ADT/DepthFirstIteratorTest.cpp b/llvm/unittests/ADT/DepthFirstIteratorTest.cpp
index 007b6839f4d05d..95923b81008389 100644
--- a/llvm/unittests/ADT/DepthFirstIteratorTest.cpp
+++ b/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);
+}
}
diff --git a/llvm/unittests/ADT/PostOrderIteratorTest.cpp b/llvm/unittests/ADT/PostOrderIteratorTest.cpp
index 17c84229cbc4d8..4c2a66e8d5b626 100644
--- a/llvm/unittests/ADT/PostOrderIteratorTest.cpp
+++ b/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);
+}
}
More information about the llvm-commits
mailing list