From d1282d80998f6436605828f60a22fce7d30f8f54 Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Wed, 19 Jun 2024 19:52:40 +0100 Subject: [PATCH] [ADT] Fix incorrect const parent ptr type in ilist (#96059) Fixes issue reported in: https://github.com/llvm/llvm-project/pull/94224 The recent commit above added an ilist_parent 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. --- llvm/include/llvm/ADT/ilist_base.h | 4 ++-- llvm/include/llvm/ADT/ilist_iterator.h | 26 +++++++++++----------- llvm/include/llvm/ADT/ilist_node.h | 18 +++++++-------- llvm/include/llvm/ADT/ilist_node_base.h | 21 +++++++++-------- llvm/include/llvm/ADT/ilist_node_options.h | 15 ++++++------- llvm/unittests/ADT/IListIteratorTest.cpp | 10 +++++++++ llvm/unittests/ADT/IListNodeBaseTest.cpp | 17 +++++++++++--- llvm/unittests/ADT/IListNodeTest.cpp | 6 ++--- 8 files changed, 68 insertions(+), 49 deletions(-) diff --git a/llvm/include/llvm/ADT/ilist_base.h b/llvm/include/llvm/ADT/ilist_base.h index 000253a26012b..4a8a556fc99be 100644 --- 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 class ilist_base { +template class ilist_base { public: - using node_base_type = ilist_node_base; + using node_base_type = ilist_node_base; 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 index 16a45b5beac50..882df9d7e767f 100644 --- a/llvm/include/llvm/ADT/ilist_iterator.h +++ b/llvm/include/llvm/ADT/ilist_iterator.h @@ -53,19 +53,19 @@ template <> struct IteratorHelper : 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 +template class iterator_parent_access; -template -class iterator_parent_access { +template +class iterator_parent_access { public: - inline const ParentPtrTy getNodeParent() const { + inline const ParentTy *getNodeParent() const { return static_cast(this)->NodePtr->getParent(); } }; -template -class iterator_parent_access { +template +class iterator_parent_access { public: - inline ParentPtrTy getNodeParent() { + inline ParentTy *getNodeParent() { return static_cast(this)->NodePtr->getParent(); } }; @@ -81,13 +81,13 @@ template class ilist_iterator : ilist_detail::SpecificNodeAccess, public ilist_detail::iterator_parent_access< ilist_iterator, - typename OptionsT::parent_ptr_ty, IsConst> { + typename OptionsT::parent_ty, IsConst> { friend ilist_iterator; friend ilist_iterator; friend ilist_iterator; friend ilist_detail::iterator_parent_access< - ilist_iterator, - typename OptionsT::parent_ptr_ty, IsConst>; + ilist_iterator, + typename OptionsT::parent_ty, IsConst>; using Traits = ilist_detail::IteratorTraits; using Access = ilist_detail::SpecificNodeAccess; @@ -215,13 +215,13 @@ class ilist_iterator_w_bits : ilist_detail::SpecificNodeAccess, public ilist_detail::iterator_parent_access< ilist_iterator_w_bits, - typename OptionsT::parent_ptr_ty, IsConst> { + typename OptionsT::parent_ty, IsConst> { friend ilist_iterator_w_bits; friend ilist_iterator_w_bits; friend ilist_iterator; friend ilist_detail::iterator_parent_access< - ilist_iterator_w_bits, - typename OptionsT::parent_ptr_ty, IsConst>; + ilist_iterator_w_bits, + typename OptionsT::parent_ty, IsConst>; using Traits = ilist_detail::IteratorTraits; using Access = ilist_detail::SpecificNodeAccess; diff --git a/llvm/include/llvm/ADT/ilist_node.h b/llvm/include/llvm/ADT/ilist_node.h index 209fd209167e8..bfd50f8b3fb71 100644 --- 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 node_parent_access { +template class node_parent_access { public: - inline const ParentPtrTy getParent() const { + inline const ParentTy *getParent() const { return static_cast(this)->getNodeBaseParent(); } - inline ParentPtrTy getParent() { + inline ParentTy *getParent() { return static_cast(this)->getNodeBaseParent(); } - void setParent(ParentPtrTy Parent) { + void setParent(ParentTy *Parent) { return static_cast(this)->setNodeBaseParent(Parent); } }; @@ -71,8 +71,8 @@ class ilist_select_iterator_type { template class ilist_node_impl : OptionsT::node_base_type, - public ilist_detail::node_parent_access< - ilist_node_impl, typename OptionsT::parent_ptr_ty> { + public ilist_detail::node_parent_access, + 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; - friend class ilist_detail::node_parent_access< - ilist_node_impl, typename OptionsT::parent_ptr_ty>; + friend class ilist_detail::node_parent_access, + typename OptionsT::parent_ty>; friend class ilist_iterator; friend class ilist_iterator; friend class ilist_iterator; diff --git a/llvm/include/llvm/ADT/ilist_node_base.h b/llvm/include/llvm/ADT/ilist_node_base.h index caad87cdd4d71..49b197d3466d9 100644 --- a/llvm/include/llvm/ADT/ilist_node_base.h +++ b/llvm/include/llvm/ADT/ilist_node_base.h @@ -46,13 +46,13 @@ template class node_base_prevnext { void initializeSentinel() { PrevAndSentinel.setInt(true); } }; -template class node_base_parent { - ParentPtrTy Parent = nullptr; +template 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 {}; @@ -61,12 +61,11 @@ template <> class node_base_parent {}; /// Base class for ilist nodes. /// /// Optionally tracks whether this node is the sentinel. -template -class ilist_node_base - : public ilist_detail::node_base_prevnext< - ilist_node_base, - EnableSentinelTracking>, - public ilist_detail::node_base_parent {}; +template +class ilist_node_base : public ilist_detail::node_base_prevnext< + ilist_node_base, + EnableSentinelTracking>, + public ilist_detail::node_base_parent {}; } // end namespace llvm diff --git a/llvm/include/llvm/ADT/ilist_node_options.h b/llvm/include/llvm/ADT/ilist_node_options.h index aa32162cd51e4..d26e79b925ad1 100644 --- 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 class ilist_node_base; -template class ilist_base; +template class ilist_node_base; +template class ilist_base; /// Option to choose whether to track sentinels. /// @@ -134,7 +134,7 @@ struct is_valid_option> : std::true_type {}; template struct extract_parent; template struct extract_parent, Options...> { - typedef ParentTy *type; + typedef ParentTy type; }; template struct extract_parent : extract_parent {}; @@ -156,7 +156,7 @@ struct check_options /// /// This is usually computed via \a compute_node_options. template + 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 - node_base_type; - typedef ilist_base list_base_type; + typedef ParentTy parent_ty; + typedef ilist_node_base node_base_type; + typedef ilist_base list_base_type; }; template struct compute_node_options { diff --git a/llvm/unittests/ADT/IListIteratorTest.cpp b/llvm/unittests/ADT/IListIteratorTest.cpp index fd420b9c82767..4e5b847b35ffe 100644 --- a/llvm/unittests/ADT/IListIteratorTest.cpp +++ b/llvm/unittests/ADT/IListIteratorTest.cpp @@ -175,6 +175,7 @@ TEST(IListIteratorTest, GetParent) { simple_ilist> L; Parent P; ParentNode A; + const simple_ilist> &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; + using ConstParentTy = + std::remove_pointer_t; + static_assert( + std::is_const_v && + std::is_same_v>, + "`getNodeParent() const` adds const to parent type"); } } // end namespace diff --git a/llvm/unittests/ADT/IListNodeBaseTest.cpp b/llvm/unittests/ADT/IListNodeBaseTest.cpp index 07f397d2ef0fe..ef90c716a4118 100644 --- 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 + using namespace llvm; namespace { @@ -17,8 +19,8 @@ class Parent {}; typedef ilist_node_base RawNode; typedef ilist_node_base TrackingNode; -typedef ilist_node_base ParentNode; -typedef ilist_node_base ParentTrackingNode; +typedef ilist_node_base ParentNode; +typedef ilist_node_base 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; + using ConstParentTy = + std::remove_pointer_t; + static_assert( + std::is_const_v && + std::is_same_v>, + "`getNodeBaseParent() const` adds const to parent type"); } } // end namespace diff --git a/llvm/unittests/ADT/IListNodeTest.cpp b/llvm/unittests/ADT/IListNodeTest.cpp index 0a5da12d7f30f..c5f39d7eb8d8d 100644 --- a/llvm/unittests/ADT/IListNodeTest.cpp +++ b/llvm/unittests/ADT/IListNodeTest.cpp @@ -66,9 +66,9 @@ TEST(IListNodeTest, Options) { ilist_sentinel_tracking>::type>, "order shouldn't matter with real tags"); static_assert( - !std::is_same_v::type, - compute_node_options>::type>, - "void parent is different to no parent"); + std::is_same_v::type, + compute_node_options>::type>, + "default parent is void"); static_assert( !std::is_same_v>::type, compute_node_options>::type>,