[PATCH] D23466: ADT: Remove UB in ilist (and use a circular linked list)

[Mehdi AMINI via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM overall, with a few inline comments that you should look at.


================
Comment at: include/llvm/ADT/ilist.h:19
@@ -18,3 +18,3 @@
 // The ilist class is implemented by allocating a 'tail' node when the list is
 // created (using ilist_traits<>::createSentinel()).  This tail node is
 // absolutely required because the user must be able to compute end()-1. Because
----------------
Stalled comment about `createSentinel()`?

================
Comment at: include/llvm/ADT/ilist.h:28-29
@@ -27,3 +27,4 @@
 #define LLVM_ADT_ILIST_H
 
+#include "llvm/ADT/ilist_node.h"
 #include "llvm/Support/Compiler.h"
----------------
It seems to be one of the main change in the model: before it was possible to create a list from something that would not inherit from ilist_node right?
I guess we don't lose much...

================
Comment at: include/llvm/ADT/ilist.h:235
@@ -310,3 +234,3 @@
     NodePtr = ilist_node_access::getPrev(NodePtr);
     assert(NodePtr && "--'d off the beginning of an ilist!");
     return *this;
----------------
Stalled assert (at least assert message)?

================
Comment at: include/llvm/ADT/ilist.h:324
@@ -424,7 +323,3 @@
 
-  iplist() : Head(this->provideInitialHead()) {}
-  ~iplist() {
-    if (!Head) return;
-    clear();
-    Traits::destroySentinel(getTail());
-  }
+  iplist() = default;
+  ~iplist() { clear(); }
----------------
Could be removed?

================
Comment at: include/llvm/ADT/ilist.h:407
@@ -530,2 +406,3 @@
     // When those iterators are incremented or decremented, they will assert on
     // the null next/prev pointer instead of "usually working".
+    this->setNext(Base, nullptr);
----------------
I'm not sure this is true, `++it` will just work fine, but it will be in a "default constructed" state (NodePtr being a nullptr).
Only `ilist_iterator &operator--()` is asserting right now (incidentally, see previous comment).


================
Comment at: include/llvm/ADT/ilist.h:470
@@ -615,1 +469,3 @@
+    // Callback.
+    this->transferNodesFromList(L2, iterator(*First), iterator(*Next));
   }
----------------
Are these different from `first` and `last`?



https://reviews.llvm.org/D23466