[ADT] Fix incorrect const parent ptr type in ilist (#96059)

SLTozer · web-flow · commit c6ed8289b7c9 · 2024-06-19T19:52:40.000+01:00
Fixes issue reported in: #94224 The recent commit above added an ilist_parent<ParentTy> option, which added a parent pointer to the ilist_node_base type for the list. The const methods for returning that parent pointer however were incorrectly implemented, returning `const ParentPtrTy`, which is equivalent to `ParentTy * const` rather than `const ParentTy *`. This patch fixes this by passing around `ParentTy` in ilist's internal logic rather than `ParentPtrTy`, removing the ability to have a `void*` parent pointer but cleanly fixing this error.
diff --git a/llvm/include/llvm/ADT/ilist_base.h b/llvm/include/llvm/ADT/ilist_base.h
@@ -15,9 +15,9 @@
 namespace llvm {
 
 /// Implementations of list algorithms using ilist_node_base.
-template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_base {
+template <bool EnableSentinelTracking, class ParentTy> class ilist_base {
 public:
-  using node_base_type = ilist_node_base<EnableSentinelTracking, ParentPtrTy>;
+  using node_base_type = ilist_node_base<EnableSentinelTracking, ParentTy>;
 
   static void insertBeforeImpl(node_base_type &Next, node_base_type &N) {
     node_base_type &Prev = *Next.getPrev();
diff --git a/llvm/include/llvm/ADT/ilist_iterator.h b/llvm/include/llvm/ADT/ilist_iterator.h
@@ -53,19 +53,19 @@ template <> struct IteratorHelper<true> : ilist_detail::NodeAccess {
 /// Mixin class used to add a \a getNodeParent() function to iterators iff the
 /// list uses \a ilist_parent, calling through to the node's \a getParent(). For
 /// more details see \a ilist_node.
-template <class IteratorTy, class ParentPtrTy, bool IsConst>
+template <class IteratorTy, class ParentTy, bool IsConst>
 class iterator_parent_access;
-template <class IteratorTy, class ParentPtrTy>
-class iterator_parent_access<IteratorTy, ParentPtrTy, true> {
+template <class IteratorTy, class ParentTy>
+class iterator_parent_access<IteratorTy, ParentTy, true> {
 public:
-  inline const ParentPtrTy getNodeParent() const {
+  inline const ParentTy *getNodeParent() const {
     return static_cast<IteratorTy *>(this)->NodePtr->getParent();
   }
 };
-template <class IteratorTy, class ParentPtrTy>
-class iterator_parent_access<IteratorTy, ParentPtrTy, false> {
+template <class IteratorTy, class ParentTy>
+class iterator_parent_access<IteratorTy, ParentTy, false> {
 public:
-  inline ParentPtrTy getNodeParent() {
+  inline ParentTy *getNodeParent() {
     return static_cast<IteratorTy *>(this)->NodePtr->getParent();
   }
 };
@@ -81,13 +81,13 @@ template <class OptionsT, bool IsReverse, bool IsConst>
 class ilist_iterator : ilist_detail::SpecificNodeAccess<OptionsT>,
                        public ilist_detail::iterator_parent_access<
                            ilist_iterator<OptionsT, IsReverse, IsConst>,
-                           typename OptionsT::parent_ptr_ty, IsConst> {
+                           typename OptionsT::parent_ty, IsConst> {
   friend ilist_iterator<OptionsT, IsReverse, !IsConst>;
   friend ilist_iterator<OptionsT, !IsReverse, IsConst>;
   friend ilist_iterator<OptionsT, !IsReverse, !IsConst>;
   friend ilist_detail::iterator_parent_access<
-                           ilist_iterator<OptionsT, IsReverse, IsConst>,
-                           typename OptionsT::parent_ptr_ty, IsConst>;
+      ilist_iterator<OptionsT, IsReverse, IsConst>,
+      typename OptionsT::parent_ty, IsConst>;
 
   using Traits = ilist_detail::IteratorTraits<OptionsT, IsConst>;
   using Access = ilist_detail::SpecificNodeAccess<OptionsT>;
@@ -215,13 +215,13 @@ class ilist_iterator_w_bits
     : ilist_detail::SpecificNodeAccess<OptionsT>,
       public ilist_detail::iterator_parent_access<
           ilist_iterator_w_bits<OptionsT, IsReverse, IsConst>,
-          typename OptionsT::parent_ptr_ty, IsConst> {
+          typename OptionsT::parent_ty, IsConst> {
   friend ilist_iterator_w_bits<OptionsT, IsReverse, !IsConst>;
   friend ilist_iterator_w_bits<OptionsT, !IsReverse, IsConst>;
   friend ilist_iterator<OptionsT, !IsReverse, !IsConst>;
   friend ilist_detail::iterator_parent_access<
-                           ilist_iterator_w_bits<OptionsT, IsReverse, IsConst>,
-                           typename OptionsT::parent_ptr_ty, IsConst>;
+      ilist_iterator_w_bits<OptionsT, IsReverse, IsConst>,
+      typename OptionsT::parent_ty, IsConst>;
 
   using Traits = ilist_detail::IteratorTraits<OptionsT, IsConst>;
   using Access = ilist_detail::SpecificNodeAccess<OptionsT>;
diff --git a/llvm/include/llvm/ADT/ilist_node.h b/llvm/include/llvm/ADT/ilist_node.h
@@ -25,17 +25,17 @@ namespace ilist_detail {
 struct NodeAccess;
 
 /// Mixin base class that is used to add \a getParent() and
-/// \a setParent(ParentPtrTy) methods to \a ilist_node_impl iff \a ilist_parent
+/// \a setParent(ParentTy*) methods to \a ilist_node_impl iff \a ilist_parent
 /// has been set in the list options.
-template <class NodeTy, class ParentPtrTy> class node_parent_access {
+template <class NodeTy, class ParentTy> class node_parent_access {
 public:
-  inline const ParentPtrTy getParent() const {
+  inline const ParentTy *getParent() const {
     return static_cast<const NodeTy *>(this)->getNodeBaseParent();
   }
-  inline ParentPtrTy getParent() {
+  inline ParentTy *getParent() {
     return static_cast<NodeTy *>(this)->getNodeBaseParent();
   }
-  void setParent(ParentPtrTy Parent) {
+  void setParent(ParentTy *Parent) {
     return static_cast<NodeTy *>(this)->setNodeBaseParent(Parent);
   }
 };
@@ -71,8 +71,8 @@ class ilist_select_iterator_type<true, Opts, arg1, arg2> {
 template <class OptionsT>
 class ilist_node_impl
     : OptionsT::node_base_type,
-      public ilist_detail::node_parent_access<
-          ilist_node_impl<OptionsT>, typename OptionsT::parent_ptr_ty> {
+      public ilist_detail::node_parent_access<ilist_node_impl<OptionsT>,
+                                              typename OptionsT::parent_ty> {
   using value_type = typename OptionsT::value_type;
   using node_base_type = typename OptionsT::node_base_type;
   using list_base_type = typename OptionsT::list_base_type;
@@ -81,8 +81,8 @@ class ilist_node_impl
   friend struct ilist_detail::NodeAccess;
   friend class ilist_sentinel<OptionsT>;
 
-  friend class ilist_detail::node_parent_access<
-      ilist_node_impl<OptionsT>, typename OptionsT::parent_ptr_ty>;
+  friend class ilist_detail::node_parent_access<ilist_node_impl<OptionsT>,
+                                                typename OptionsT::parent_ty>;
   friend class ilist_iterator<OptionsT, false, false>;
   friend class ilist_iterator<OptionsT, false, true>;
   friend class ilist_iterator<OptionsT, true, false>;
diff --git a/llvm/include/llvm/ADT/ilist_node_base.h b/llvm/include/llvm/ADT/ilist_node_base.h
@@ -46,13 +46,13 @@ template <class NodeBase> class node_base_prevnext<NodeBase, true> {
   void initializeSentinel() { PrevAndSentinel.setInt(true); }
 };
 
-template <class ParentPtrTy> class node_base_parent {
-  ParentPtrTy Parent = nullptr;
+template <class ParentTy> class node_base_parent {
+  ParentTy *Parent = nullptr;
 
 public:
-  void setNodeBaseParent(ParentPtrTy Parent) { this->Parent = Parent; }
-  inline const ParentPtrTy getNodeBaseParent() const { return Parent; }
-  inline ParentPtrTy getNodeBaseParent() { return Parent; }
+  void setNodeBaseParent(ParentTy *Parent) { this->Parent = Parent; }
+  inline const ParentTy *getNodeBaseParent() const { return Parent; }
+  inline ParentTy *getNodeBaseParent() { return Parent; }
 };
 template <> class node_base_parent<void> {};
 
@@ -61,12 +61,11 @@ template <> class node_base_parent<void> {};
 /// Base class for ilist nodes.
 ///
 /// Optionally tracks whether this node is the sentinel.
-template <bool EnableSentinelTracking, class ParentPtrTy>
-class ilist_node_base
-    : public ilist_detail::node_base_prevnext<
-          ilist_node_base<EnableSentinelTracking, ParentPtrTy>,
-          EnableSentinelTracking>,
-      public ilist_detail::node_base_parent<ParentPtrTy> {};
+template <bool EnableSentinelTracking, class ParentTy>
+class ilist_node_base : public ilist_detail::node_base_prevnext<
+                            ilist_node_base<EnableSentinelTracking, ParentTy>,
+                            EnableSentinelTracking>,
+                        public ilist_detail::node_base_parent<ParentTy> {};
 
 } // end namespace llvm
 
diff --git a/llvm/include/llvm/ADT/ilist_node_options.h b/llvm/include/llvm/ADT/ilist_node_options.h
@@ -15,8 +15,8 @@
 
 namespace llvm {
 
-template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_node_base;
-template <bool EnableSentinelTracking, class ParentPtrTy> class ilist_base;
+template <bool EnableSentinelTracking, class ParentTy> class ilist_node_base;
+template <bool EnableSentinelTracking, class ParentTy> class ilist_base;
 
 /// Option to choose whether to track sentinels.
 ///
@@ -134,7 +134,7 @@ struct is_valid_option<ilist_iterator_bits<IteratorBits>> : std::true_type {};
 template <class... Options> struct extract_parent;
 template <class ParentTy, class... Options>
 struct extract_parent<ilist_parent<ParentTy>, Options...> {
-  typedef ParentTy *type;
+  typedef ParentTy type;
 };
 template <class Option1, class... Options>
 struct extract_parent<Option1, Options...> : extract_parent<Options...> {};
@@ -156,7 +156,7 @@ struct check_options<Option1, Options...>
 ///
 /// This is usually computed via \a compute_node_options.
 template <class T, bool EnableSentinelTracking, bool IsSentinelTrackingExplicit,
-          class TagT, bool HasIteratorBits, class ParentPtrTy>
+          class TagT, bool HasIteratorBits, class ParentTy>
 struct node_options {
   typedef T value_type;
   typedef T *pointer;
@@ -168,10 +168,9 @@ struct node_options {
   static const bool is_sentinel_tracking_explicit = IsSentinelTrackingExplicit;
   static const bool has_iterator_bits = HasIteratorBits;
   typedef TagT tag;
-  typedef ParentPtrTy parent_ptr_ty;
-  typedef ilist_node_base<enable_sentinel_tracking, parent_ptr_ty>
-      node_base_type;
-  typedef ilist_base<enable_sentinel_tracking, parent_ptr_ty> list_base_type;
+  typedef ParentTy parent_ty;
+  typedef ilist_node_base<enable_sentinel_tracking, parent_ty> node_base_type;
+  typedef ilist_base<enable_sentinel_tracking, parent_ty> list_base_type;
 };
 
 template <class T, class... Options> struct compute_node_options {
diff --git a/llvm/unittests/ADT/IListIteratorTest.cpp b/llvm/unittests/ADT/IListIteratorTest.cpp
@@ -175,6 +175,7 @@ TEST(IListIteratorTest, GetParent) {
   simple_ilist<ParentNode, ilist_parent<Parent>> L;
   Parent P;
   ParentNode A;
+  const simple_ilist<ParentNode, ilist_parent<Parent>> &CL = L;
 
   // Parents are not set automatically.
   A.setParent(&P);
@@ -187,6 +188,15 @@ TEST(IListIteratorTest, GetParent) {
   EXPECT_EQ(&P, L.end().getNodeParent());
   EXPECT_EQ(&P, L.rbegin().getNodeParent());
   EXPECT_EQ(&P, L.rend().getNodeParent());
+
+  using VarParentTy =
+      std::remove_pointer_t<decltype(L.begin().getNodeParent())>;
+  using ConstParentTy =
+      std::remove_pointer_t<decltype(CL.begin().getNodeParent())>;
+  static_assert(
+      std::is_const_v<ConstParentTy> &&
+          std::is_same_v<VarParentTy, std::remove_const_t<ConstParentTy>>,
+      "`getNodeParent() const` adds const to parent type");
 }
 
 } // end namespace
diff --git a/llvm/unittests/ADT/IListNodeBaseTest.cpp b/llvm/unittests/ADT/IListNodeBaseTest.cpp
@@ -9,6 +9,8 @@
 #include "llvm/ADT/ilist_node_base.h"
 #include "gtest/gtest.h"
 
+#include <type_traits>
+
 using namespace llvm;
 
 namespace {
@@ -17,8 +19,8 @@ class Parent {};
 
 typedef ilist_node_base<false, void> RawNode;
 typedef ilist_node_base<true, void> TrackingNode;
-typedef ilist_node_base<false, Parent*> ParentNode;
-typedef ilist_node_base<true, Parent*> ParentTrackingNode;
+typedef ilist_node_base<false, Parent> ParentNode;
+typedef ilist_node_base<true, Parent> ParentTrackingNode;
 
 TEST(IListNodeBaseTest, DefaultConstructor) {
   RawNode A;
@@ -177,9 +179,10 @@ TEST(IListNodeBaseTest, isKnownSentinel) {
   EXPECT_EQ(&PTB, PTA.getNext());
 }
 
-TEST(IListNodeBaseTest, setNodeBaseParent) {
+TEST(IListNodeBaseTest, nodeBaseParent) {
   Parent Par;
   ParentNode PA;
+  const ParentNode CPA;
   EXPECT_EQ(nullptr, PA.getNodeBaseParent());
   PA.setNodeBaseParent(&Par);
   EXPECT_EQ(&Par, PA.getNodeBaseParent());
@@ -188,6 +191,14 @@ TEST(IListNodeBaseTest, setNodeBaseParent) {
   EXPECT_EQ(nullptr, PTA.getNodeBaseParent());
   PTA.setNodeBaseParent(&Par);
   EXPECT_EQ(&Par, PTA.getNodeBaseParent());
+
+  using VarParentTy = std::remove_pointer_t<decltype(PA.getNodeBaseParent())>;
+  using ConstParentTy =
+      std::remove_pointer_t<decltype(CPA.getNodeBaseParent())>;
+  static_assert(
+      std::is_const_v<ConstParentTy> &&
+          std::is_same_v<VarParentTy, std::remove_const_t<ConstParentTy>>,
+      "`getNodeBaseParent() const` adds const to parent type");
 }
 
 } // end namespace
diff --git a/llvm/unittests/ADT/IListNodeTest.cpp b/llvm/unittests/ADT/IListNodeTest.cpp
@@ -66,9 +66,9 @@ TEST(IListNodeTest, Options) {
                                           ilist_sentinel_tracking<true>>::type>,
       "order shouldn't matter with real tags");
   static_assert(
-      !std::is_same_v<compute_node_options<Node>::type,
-                      compute_node_options<Node, ilist_parent<void>>::type>,
-      "void parent is different to no parent");
+      std::is_same_v<compute_node_options<Node>::type,
+                     compute_node_options<Node, ilist_parent<void>>::type>,
+      "default parent is void");
   static_assert(
       !std::is_same_v<compute_node_options<Node, ilist_parent<ParentA>>::type,
                       compute_node_options<Node, ilist_parent<void>>::type>,
