[llvm] c6ed828 - [ADT] Fix incorrect const parent ptr type in ilist (#96059)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 19 11:52:43 PDT 2024
Author: Stephen Tozer
Date: 2024-06-19T19:52:40+01:00
New Revision: c6ed8289b7c948464855841632f6b6783da1b65a
URL: https://github.com/llvm/llvm-project/commit/c6ed8289b7c948464855841632f6b6783da1b65a
DIFF: https://github.com/llvm/llvm-project/commit/c6ed8289b7c948464855841632f6b6783da1b65a.diff
LOG: [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<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.
Added:
Modified:
llvm/include/llvm/ADT/ilist_base.h
llvm/include/llvm/ADT/ilist_iterator.h
llvm/include/llvm/ADT/ilist_node.h
llvm/include/llvm/ADT/ilist_node_base.h
llvm/include/llvm/ADT/ilist_node_options.h
llvm/unittests/ADT/IListIteratorTest.cpp
llvm/unittests/ADT/IListNodeBaseTest.cpp
llvm/unittests/ADT/IListNodeTest.cpp
Removed:
################################################################################
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/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<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
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 <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
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
diff erent 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>,
More information about the llvm-commits
mailing list