[ADT] Fix incorrect const parent ptr type in ilist #96059
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes issue reported in: llvm#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.
|
@llvm/pr-subscribers-llvm-adt Author: Stephen Tozer (SLTozer) ChangesFixes 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 Full diff: https://github.com/llvm/llvm-project/pull/96059.diff 7 Files Affected:
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 <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
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<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
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 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
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 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
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 <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/IListNodeBaseTest.cpp b/llvm/unittests/ADT/IListNodeBaseTest.cpp
index 07f397d2ef0fe..2d44e320d42ed 100644
--- a/llvm/unittests/ADT/IListNodeBaseTest.cpp
+++ b/llvm/unittests/ADT/IListNodeBaseTest.cpp
@@ -17,8 +17,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;
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<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>,
|
nikic
approved these changes
Jun 19, 2024
|
Added a bit of testing - not sure exactly how much is right for this, but I assume it doesn't need to be too thorough. |
nikic
approved these changes
Jun 19, 2024
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
Fixes issue reported in: llvm#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue reported in: #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 toParentTy * constrather thanconst ParentTy *. This patch fixes this by passing aroundParentTyin ilist's internal logic rather thanParentPtrTy, removing the ability to have avoid*parent pointer but cleanly fixing this error.