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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Aug-12, at 15:36, Mehdi AMINI <mehdi.amini at apple.com> wrote:
> 
> 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.

Thanks for the review!  Responding to comments now, but I'll wait to commit until I've gotten the Windows bots green again (the essentially NFC prep commit r278532-9 exposed a bunch of *end() calls in X86, since now they crash on Windows).

> ================
> 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()`?

Yes, it's stale.

> ================
> 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...

I already broke this when I killed ilist_nextprev_traits, but yes, that is a change.  Since no one was using this for anything good (SimpleReference in lld was using it, but not doing anything interesting with it), I didn't think to call it out.

> ================
> 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)?

Right, not relevant.

> ================
> 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?

Certainly the destructor needs to stay.  I'll remove the default constructor if it still compiles (but I'm always wary about what will or will not compile on MSVC...)

> ================
> 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).

I don't think this old comment is related to the assertion in getNextNode.  Incrementing or decrementing the pointer will make it so that any of the following should crash: ++it, --it, *it.  As you point out, that's the same as a default-constructed ilist_iterator<T>, but I still think it's valuable.

> ================
> 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`?

Actually, these are equivalent to `first` and `where`.  I'll be ripping this out into an untemplated ilist_base in a follow-up; I'll reconsider the variable choice then (probably at least deserves a comment...).

> 
> 
> 
> https://reviews.llvm.org/D23466
> 
> 
>