ADT: Split out simple_ilist, a simple intrusive list

Split out a new, low-level intrusive list type with clear semantics.
Unlike iplist (and ilist), all operations on simple_ilist are intrusive,
and simple_ilist never takes ownership of its nodes.  This enables an
intuitive API that has the right defaults for intrusive lists.
- insert() takes references (not pointers!) to nodes (in iplist/ilist,
  passing a reference will cause the node to be copied).
- erase() takes only iterators (like std::list), and does not destroy
  the nodes.
- remove() takes only references and has the same behaviour as erase().
- clear() does not destroy the nodes.
- The destructor does not destroy the nodes.
- New API {erase,remove,clear}AndDispose() take an extra Disposer
  functor for callsites that want to call some disposal routine (e.g.,
  std::default_delete).

This list is not currently configurable, and has no callbacks.

The initial motivation was to fix iplist<>::sort to work correctly (even
with callbacks in ilist_traits<>).  iplist<> uses simple_ilist<>::sort
directly.  The new test in unittests/IR/ModuleTest.cpp crashes without
this commit.

Fixing sort() via a low-level layer provided a good opportunity to:
- Unit test the low-level functionality thoroughly.
- Modernize the API, largely inspired by other intrusive list
  implementations.

Here's a sketch of a longer-term plan:
- Create BumpPtrList<>, a non-intrusive list implemented using
  simple_ilist<>, and use it for the Token list in
  lib/Support/YAMLParser.cpp.  This will factor out the only real use of
  createNode().
- Evolve the iplist<> and ilist<> APIs in the direction of
  simple_ilist<>, making allocation/deallocation explicit at call sites
  (similar to simple_ilist<>::eraseAndDispose()).
- Factor out remaining calls to createNode() and deleteNode() and remove
  the customization from ilist_traits<>.
- Transition uses of iplist<>/ilist<> that don't need callbacks over to
  simple_ilist<>.

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

Author: dexonsmith

include/llvm/ADT/ilist.h

@@ -24,11 +24,8 @@
 #ifndef LLVM_ADT_ILIST_H
 #define LLVM_ADT_ILIST_H
 
-#include "llvm/ADT/ilist_base.h"
-#include "llvm/ADT/ilist_iterator.h"
-#include "llvm/ADT/ilist_node.h"
+#include "llvm/ADT/simple_ilist.h"
 #include "llvm/Support/Compiler.h"
-#include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <iterator>
@@ -119,17 +116,14 @@ struct ilist_traits<const Ty> : public ilist_traits<Ty> {};
 /// ilist_sentinel, which holds pointers to the first and last nodes in the
 /// list.
 template <typename NodeTy, typename Traits = ilist_traits<NodeTy>>
-class iplist : public Traits, ilist_base, ilist_node_access {
+class iplist : public Traits, simple_ilist<NodeTy> {
   // TODO: Drop this assertion and the transitive type traits anytime after
   // v4.0 is branched (i.e,. keep them for one release to help out-of-tree code
   // update).
   static_assert(!ilist_detail::HasObsoleteCustomization<Traits, NodeTy>::value,
                 "ilist customization points have changed!");
 
-  ilist_sentinel<NodeTy> Sentinel;
-
-  typedef ilist_node<NodeTy> node_type;
-  typedef const ilist_node<NodeTy> const_node_type;
+  typedef simple_ilist<NodeTy> base_list_type;
 
   static bool op_less(NodeTy &L, NodeTy &R) { return L < R; }
   static bool op_equal(NodeTy &L, NodeTy &R) { return L == R; }
@@ -139,65 +133,42 @@ class iplist : public Traits, ilist_base, ilist_node_access {
   void operator=(const iplist &) = delete;
 
 public:
-  typedef NodeTy *pointer;
-  typedef const NodeTy *const_pointer;
-  typedef NodeTy &reference;
-  typedef const NodeTy &const_reference;
-  typedef NodeTy value_type;
-  typedef ilist_iterator<NodeTy> iterator;
-  typedef ilist_iterator<const NodeTy> const_iterator;
-  typedef size_t size_type;
-  typedef ptrdiff_t difference_type;
-  typedef ilist_iterator<const NodeTy, true> const_reverse_iterator;
-  typedef ilist_iterator<NodeTy, true> reverse_iterator;
+  typedef typename base_list_type::pointer pointer;
+  typedef typename base_list_type::const_pointer const_pointer;
+  typedef typename base_list_type::reference reference;
+  typedef typename base_list_type::const_reference const_reference;
+  typedef typename base_list_type::value_type value_type;
+  typedef typename base_list_type::size_type size_type;
+  typedef typename base_list_type::difference_type difference_type;
+  typedef typename base_list_type::iterator iterator;
+  typedef typename base_list_type::const_iterator const_iterator;
+  typedef typename base_list_type::reverse_iterator reverse_iterator;
+  typedef
+      typename base_list_type::const_reverse_iterator const_reverse_iterator;
 
   iplist() = default;
   ~iplist() { clear(); }
 
-  // Iterator creation methods.
-  iterator begin() { return ++iterator(Sentinel); }
-  const_iterator begin() const { return ++const_iterator(Sentinel); }
-  iterator end() { return iterator(Sentinel); }
-  const_iterator end() const { return const_iterator(Sentinel); }
-
-  // reverse iterator creation methods.
-  reverse_iterator rbegin()            { return ++reverse_iterator(Sentinel); }
-  const_reverse_iterator rbegin() const{ return ++const_reverse_iterator(Sentinel); }
-  reverse_iterator rend()              { return reverse_iterator(Sentinel); }
-  const_reverse_iterator rend() const { return const_reverse_iterator(Sentinel); }
-
   // Miscellaneous inspection routines.
   size_type max_size() const { return size_type(-1); }
-  bool LLVM_ATTRIBUTE_UNUSED_RESULT empty() const { return Sentinel.empty(); }
 
-  // Front and back accessor functions...
-  reference front() {
-    assert(!empty() && "Called front() on empty list!");
-    return *begin();
-  }
-  const_reference front() const {
-    assert(!empty() && "Called front() on empty list!");
-    return *begin();
-  }
-  reference back() {
-    assert(!empty() && "Called back() on empty list!");
-    return *--end();
-  }
-  const_reference back() const {
-    assert(!empty() && "Called back() on empty list!");
-    return *--end();
-  }
+  using base_list_type::begin;
+  using base_list_type::end;
+  using base_list_type::rbegin;
+  using base_list_type::rend;
+  using base_list_type::empty;
+  using base_list_type::front;
+  using base_list_type::back;
 
   void swap(iplist &RHS) {
     assert(0 && "Swap does not use list traits callback correctly yet!");
-    std::swap(Sentinel, RHS.Sentinel);
+    base_list_type::swap(RHS);
   }
 
   iterator insert(iterator where, NodeTy *New) {
-    ilist_base::insertBefore(*where.getNodePtr(), *this->getNodePtr(New));
-
+    auto I = base_list_type::insert(where, *New);
     this->addNodeToList(New);  // Notify traits that we added a node...
-    return iterator(New);
+    return I;
   }
 
   iterator insert(iterator where, const NodeTy &New) {
@@ -212,9 +183,8 @@ class iplist : public Traits, ilist_base, ilist_node_access {
   }
 
   NodeTy *remove(iterator &IT) {
-    assert(IT != end() && "Cannot remove end of list!");
-    NodeTy *Node = &*IT++;
-    ilist_base::remove(*this->getNodePtr(Node));
+    NodeTy *Node = &*IT;
+    base_list_type::erase(IT++);
     this->removeNodeFromList(Node);  // Notify traits that we removed a node...
     return Node;
   }
@@ -241,7 +211,7 @@ class iplist : public Traits, ilist_base, ilist_node_access {
   ///
   /// This should only be used immediately before freeing nodes in bulk to
   /// avoid traversing the list and bringing all the nodes into cache.
-  void clearAndLeakNodesUnsafely() { Sentinel.reset(); }
+  void clearAndLeakNodesUnsafely() { base_list_type::clear(); }
 
 private:
   // transfer - The heart of the splice function.  Move linked list nodes from
@@ -251,8 +221,7 @@ class iplist : public Traits, ilist_base, ilist_node_access {
     if (position == last)
       return;
 
-    ilist_base::transferBefore(*position.getNodePtr(), *first.getNodePtr(),
-                               *last.getNodePtr());
+    base_list_type::splice(position, L2, first, last);
 
     // Callback.  Note that the nodes have moved from before-last to
     // before-position.
@@ -265,9 +234,7 @@ class iplist : public Traits, ilist_base, ilist_node_access {
   // Functionality derived from other functions defined above...
   //
 
-  size_type LLVM_ATTRIBUTE_UNUSED_RESULT size() const {
-    return std::distance(begin(), end());
-  }
+  using base_list_type::size;
 
   iterator erase(iterator first, iterator last) {
     while (first != last)
@@ -318,48 +285,12 @@ class iplist : public Traits, ilist_base, ilist_node_access {
   void merge(iplist &Right, Compare comp) {
     if (this == &Right)
       return;
-    iterator First1 = begin(), Last1 = end();
-    iterator First2 = Right.begin(), Last2 = Right.end();
-    while (First1 != Last1 && First2 != Last2) {
-      if (comp(*First2, *First1)) {
-        iterator Next = First2;
-        transfer(First1, Right, First2, ++Next);
-        First2 = Next;
-      } else {
-        ++First1;
-      }
-    }
-    if (First2 != Last2)
-      transfer(Last1, Right, First2, Last2);
+    this->transferNodesFromList(Right, Right.begin(), Right.end());
+    base_list_type::merge(Right, comp);
   }
   void merge(iplist &Right) { return merge(Right, op_less); }
 
-  template <class Compare>
-  void sort(Compare comp) {
-    // The list is empty, vacuously sorted.
-    if (empty())
-      return;
-    // The list has a single element, vacuously sorted.
-    if (std::next(begin()) == end())
-      return;
-    // Find the split point for the list.
-    iterator Center = begin(), End = begin();
-    while (End != end() && std::next(End) != end()) {
-      Center = std::next(Center);
-      End = std::next(std::next(End));
-    }
-    // Split the list into two.
-    iplist RightHalf;
-    RightHalf.splice(RightHalf.begin(), *this, Center, end());
-
-    // Sort the two sublists.
-    sort(comp);
-    RightHalf.sort(comp);
-
-    // Merge the two sublists back together.
-    merge(RightHalf, comp);
-  }
-  void sort() { sort(op_less); }
+  using base_list_type::sort;
 
   /// \brief Get the previous node, or \c nullptr for the list head.
   NodeTy *getPrevNode(NodeTy &N) const {

include/llvm/ADT/ilist_base.h

@@ -39,10 +39,22 @@ class ilist_base {
     N.setNext(nullptr);
   }
 
+  static void removeRangeImpl(ilist_node_base &First, ilist_node_base &Last) {
+    ilist_node_base *Prev = First.getPrev();
+    ilist_node_base *Final = Last.getPrev();
+    Last.setPrev(Prev);
+    Prev->setNext(&Last);
+
+    // Not strictly necessary, but helps catch a class of bugs.
+    First.setPrev(nullptr);
+    Final->setNext(nullptr);
+  }
+
   static void transferBeforeImpl(ilist_node_base &Next, ilist_node_base &First,
                                  ilist_node_base &Last) {
-    assert(&Next != &Last && "Should be checked by callers");
-    assert(&First != &Last && "Should be checked by callers");
+    if (&Next == &Last || &First == &Last)
+      return;
+
     // Position cannot be contained in the range to be transferred.
     assert(&Next != &First &&
            // Check for the most common mistake.
@@ -67,6 +79,9 @@ class ilist_base {
   }
 
   template <class T> static void remove(T &N) { removeImpl(N); }
+  template <class T> static void removeRange(T &First, T &Last) {
+    removeRangeImpl(First, Last);
+  }
 
   template <class T> static void transferBefore(T &Next, T &First, T &Last) {
     transferBeforeImpl(Next, First, Last);