ADT: Never allocate nodes in iplist<> and ilist<>

Remove createNode() and any API that depending on it, and add
HasCreateNode to the list of checks for HasObsoleteCustomizations.  Now
an ilist *never* allocates (this was already true for iplist).

This factors out all the differences between iplist and ilist.  I'll aim
to rename both to "owning_ilist" eventually, to call out the interesting
(not exactly intrusive) ownership semantics.  In the meantime, I've left
both names around to reduce code churn.

One of the deleted APIs is the ilist copy constructor.  I've lifted up
and tested iplist::cloneFrom (ala simple_ilist::cloneFrom) as a
replacement.

Users of ilist<> and iplist<> that want the list to allocate nodes have
a few options:
- use std::list;
- use AllocatorList or BumpPtrList (or build a similarly trivial list);
- use cloneFrom (which is explicit at the call site); or
- allocate at the call site.

See r280573, r281177, r281181, and r281182 for examples of what to do if
you're updating out-of-tree code.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@281184 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/ADT/ilist.h

@@ -33,25 +33,24 @@
 
 namespace llvm {
 
-/// Use new/delete by default for iplist and ilist.
+/// Use delete by default for iplist and ilist.
 ///
-/// Specialize this to get different behaviour for allocation-related API.  (If
-/// you really want new/delete, consider just using std::list.)
+/// Specialize this to get different behaviour for ownership-related API.  (If
+/// you really want ownership semantics, consider using std::list or building
+/// something like \a BumpPtrList.)
 ///
 /// \see ilist_noalloc_traits
 template <typename NodeTy> struct ilist_alloc_traits {
-  /// Clone a node.
-  ///
-  /// TODO: Remove this and API that relies on it (it's dead code).
-  static NodeTy *createNode(const NodeTy &V) { return new NodeTy(V); }
   static void deleteNode(NodeTy *V) { delete V; }
 };
 
-/// Custom traits to disable node creation and do nothing on deletion.
+/// Custom traits to do nothing on deletion.
 ///
 /// Specialize ilist_alloc_traits to inherit from this to disable the
-/// non-intrusive parts of iplist and/or ilist.  It has no createNode function,
-/// and deleteNode does nothing.
+/// non-intrusive deletion in iplist (which implies ownership).
+///
+/// If you want purely intrusive semantics with no callbacks, consider using \a
+/// simple_ilist instead.
 ///
 /// \code
 /// template <>
@@ -139,9 +138,26 @@ template <class TraitsT> struct HasCreateSentinel {
   static const bool value = sizeof(test<TraitsT>(nullptr)) == sizeof(Yes);
 };
 
+/// Type trait to check for a traits class that has a createNode member.
+/// Allocation should be managed in a wrapper class, instead of in
+/// ilist_traits.
+template <class TraitsT, class NodeT> struct HasCreateNode {
+  typedef char Yes[1];
+  typedef char No[2];
+  template <size_t N> struct SFINAE {};
+
+  template <class U>
+  static Yes &test(U *I, decltype(I->createNode(make<NodeT>())) * = 0);
+  template <class> static No &test(...);
+
+public:
+  static const bool value = sizeof(test<TraitsT>(nullptr)) == sizeof(Yes);
+};
+
 template <class TraitsT, class NodeT> struct HasObsoleteCustomization {
-  static const bool value =
-      HasGetNext<TraitsT, NodeT>::value || HasCreateSentinel<TraitsT>::value;
+  static const bool value = HasGetNext<TraitsT, NodeT>::value ||
+                            HasCreateSentinel<TraitsT>::value ||
+                            HasCreateNode<TraitsT, NodeT>::value;
 };
 
 } // end namespace ilist_detail
@@ -239,6 +255,13 @@ class iplist_impl : public TraitsT, IntrusiveListT {
       return insert(++where, New);
   }
 
+  /// Clone another list.
+  template <class Cloner> void cloneFrom(const iplist_impl &L2, Cloner clone) {
+    clear();
+    for (const_reference V : L2)
+      push_back(clone(V));
+  }
+
   pointer remove(iterator &IT) {
     pointer Node = &*IT++;
     this->removeNodeFromList(Node); // Notify traits that we removed a node...
@@ -394,91 +417,7 @@ class iplist
   iplist &operator=(const iplist &X) = delete;
 };
 
-/// An intrusive list with ownership and callbacks specified/controlled by
-/// ilist_traits, with API that is unsafe for polymorphic types.
-template <class T, class... Options>
-class ilist : public iplist<T, Options...> {
-  typedef iplist<T, Options...> base_list_type;
-
-public:
-  typedef typename base_list_type::size_type size_type;
-  typedef typename base_list_type::iterator iterator;
-  typedef typename base_list_type::value_type value_type;
-  typedef typename base_list_type::const_reference const_reference;
-
-  ilist() {}
-  ilist(const ilist &right) : base_list_type() {
-    insert(this->begin(), right.begin(), right.end());
-  }
-  explicit ilist(size_type count) {
-    insert(this->begin(), count, value_type());
-  }
-  ilist(size_type count, const_reference val) {
-    insert(this->begin(), count, val);
-  }
-  template<class InIt> ilist(InIt first, InIt last) {
-    insert(this->begin(), first, last);
-  }
-
-  ilist(ilist &&X) : base_list_type(std::move(X)) {}
-  ilist &operator=(ilist &&X) {
-    *static_cast<base_list_type *>(this) = std::move(X);
-    return *this;
-  }
-
-  // bring hidden functions into scope
-  using base_list_type::insert;
-  using base_list_type::push_front;
-  using base_list_type::push_back;
-
-  // Main implementation here - Insert for a node passed by value...
-  iterator insert(iterator where, const_reference val) {
-    return insert(where, this->createNode(val));
-  }
-
-
-  // Front and back inserters...
-  void push_front(const_reference val) { insert(this->begin(), val); }
-  void push_back(const_reference val) { insert(this->end(), val); }
-
-  void insert(iterator where, size_type count, const_reference val) {
-    for (; count != 0; --count) insert(where, val);
-  }
-
-  // Assign special forms...
-  void assign(size_type count, const_reference val) {
-    iterator I = this->begin();
-    for (; I != this->end() && count != 0; ++I, --count)
-      *I = val;
-    if (count != 0)
-      insert(this->end(), val, val);
-    else
-      erase(I, this->end());
-  }
-  template<class InIt> void assign(InIt first1, InIt last1) {
-    iterator first2 = this->begin(), last2 = this->end();
-    for ( ; first1 != last1 && first2 != last2; ++first1, ++first2)
-      *first1 = *first2;
-    if (first2 == last2)
-      erase(first1, last1);
-    else
-      insert(last1, first2, last2);
-  }
-
-
-  // Resize members...
-  void resize(size_type newsize, value_type val) {
-    iterator i = this->begin();
-    size_type len = 0;
-    for ( ; i != this->end() && len < newsize; ++i, ++len) /* empty*/ ;
-
-    if (len == newsize)
-      erase(i, this->end());
-    else                                          // i == end()
-      insert(this->end(), newsize - len, val);
-  }
-  void resize(size_type newsize) { resize(newsize, value_type()); }
-};
+template <class T, class... Options> using ilist = iplist<T, Options...>;
 
 } // End llvm namespace
 

include/llvm/CodeGen/MachineBasicBlock.h

@@ -53,7 +53,6 @@ template <> struct ilist_traits<MachineInstr> {
                              instr_iterator Last);
 
   void deleteNode(MachineInstr *MI);
-  // Leave out createNode...
 };
 
 class MachineBasicBlock

include/llvm/CodeGen/MachineFunction.h

@@ -50,7 +50,6 @@ struct WinEHFuncInfo;
 
 template <> struct ilist_alloc_traits<MachineBasicBlock> {
   void deleteNode(MachineBasicBlock *MBB);
-  // Disallow createNode...
 };
 
 template <> struct ilist_callback_traits<MachineBasicBlock> {

include/llvm/CodeGen/SelectionDAG.h

@@ -85,7 +85,6 @@ template <> struct ilist_alloc_traits<SDNode> {
   static void deleteNode(SDNode *) {
     llvm_unreachable("ilist_traits<SDNode> shouldn't see a deleteNode call!");
   }
-  // Don't implement createNode...
 };
 
 /// Keeps track of dbg_value information through SDISel.  We do

include/llvm/MC/MCSection.h

@@ -33,7 +33,6 @@ class raw_ostream;
 
 template <> struct ilist_alloc_traits<MCFragment> {
   static void deleteNode(MCFragment *V);
-  // Leave out createNode...
 };
 
 /// Instances of this class represent a uniqued identifier for a section in the

unittests/ADT/IListTest.cpp

@@ -28,12 +28,12 @@ struct Node : ilist_node<Node> {
 
 TEST(IListTest, Basic) {
   ilist<Node> List;
-  List.push_back(Node(1));
+  List.push_back(new Node(1));
   EXPECT_EQ(1, List.back().Value);
   EXPECT_EQ(nullptr, List.getPrevNode(List.back()));
   EXPECT_EQ(nullptr, List.getNextNode(List.back()));
 
-  List.push_back(Node(2));
+  List.push_back(new Node(2));
   EXPECT_EQ(2, List.back().Value);
   EXPECT_EQ(2, List.getNextNode(List.front())->Value);
   EXPECT_EQ(1, List.getPrevNode(List.back())->Value);
@@ -44,9 +44,40 @@ TEST(IListTest, Basic) {
   EXPECT_EQ(1, ConstList.getPrevNode(ConstList.back())->Value);
 }
 
+TEST(IListTest, cloneFrom) {
+  Node L1Nodes[] = {Node(0), Node(1)};
+  Node L2Nodes[] = {Node(0), Node(1)};
+  ilist<Node> L1, L2, L3;
+
+  // Build L1 from L1Nodes.
+  L1.push_back(&L1Nodes[0]);
+  L1.push_back(&L1Nodes[1]);
+
+  // Build L2 from L2Nodes, based on L1 nodes.
+  L2.cloneFrom(L1, [&](const Node &N) { return &L2Nodes[N.Value]; });
+
+  // Add a node to L3 to be deleted, and then rebuild L3 by copying L1.
+  L3.push_back(new Node(7));
+  L3.cloneFrom(L1, [](const Node &N) { return new Node(N); });
+
+  EXPECT_EQ(2u, L1.size());
+  EXPECT_EQ(&L1Nodes[0], &L1.front());
+  EXPECT_EQ(&L1Nodes[1], &L1.back());
+  EXPECT_EQ(2u, L2.size());
+  EXPECT_EQ(&L2Nodes[0], &L2.front());
+  EXPECT_EQ(&L2Nodes[1], &L2.back());
+  EXPECT_EQ(2u, L3.size());
+  EXPECT_EQ(0, L3.front().Value);
+  EXPECT_EQ(1, L3.back().Value);
+
+  // Don't free nodes on the stack.
+  L1.clearAndLeakNodesUnsafely();
+  L2.clearAndLeakNodesUnsafely();
+}
+
 TEST(IListTest, SpliceOne) {
   ilist<Node> List;
-  List.push_back(1);
+  List.push_back(new Node(1));
 
   // The single-element splice operation supports noops.
   List.splice(List.begin(), List, List.begin());
@@ -55,8 +86,8 @@ TEST(IListTest, SpliceOne) {
   EXPECT_TRUE(std::next(List.begin()) == List.end());
 
   // Altenative noop. Move the first element behind itself.
-  List.push_back(2);
-  List.push_back(3);
+  List.push_back(new Node(2));
+  List.push_back(new Node(3));
   List.splice(std::next(List.begin()), List, List.begin());
   EXPECT_EQ(3u, List.size());
   EXPECT_EQ(1, List.front().Value);
@@ -111,7 +142,7 @@ TEST(IListTest, UnsafeClear) {
   EXPECT_TRUE(E == List.end());
 
   // List with contents.
-  List.push_back(1);
+  List.push_back(new Node(1));
   ASSERT_EQ(1u, List.size());
   Node *N = &*List.begin();
   EXPECT_EQ(1, N->Value);
@@ -121,8 +152,8 @@ TEST(IListTest, UnsafeClear) {
   delete N;
 
   // List is still functional.
-  List.push_back(5);
-  List.push_back(6);
+  List.push_back(new Node(5));
+  List.push_back(new Node(6));
   ASSERT_EQ(2u, List.size());
   EXPECT_EQ(5, List.front().Value);
   EXPECT_EQ(6, List.back().Value);