<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div><br></div><div><br>On Jul 12, 2016, at 00:47, Ulrich Weigand <<a href="mailto:Ulrich.Weigand@de.ibm.com">Ulrich.Weigand@de.ibm.com</a>> wrote:<br><br></div><blockquote type="cite"><div><p><tt><a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a> wrote on 11.07.2016 23:13:17:<br><br>> See PR26753 and "ilist/iplist are broken (maybe I'll fix them?)":<br>> <a href="http://lists.llvm.org/pipermail/llvm-dev/2015-October/091115.html">http://lists.llvm.org/pipermail/llvm-dev/2015-October/091115.html</a><br>> <br>> I thought I was "finished" back in November, but I'd forgotten about<br>> MachineInstrBundleIterator (MachineBasicBlock::iterator) and then <br>> didn't find time to continue until the last few weeks.<br>> <br>> Let me summarize the big picture (adding llvm-dev since I haven't <br>> talked about this in a while):<br>> - List iterators work by having a pointer to a list node and walking<br>> "node->next" every time operator++ is called.<br>> - Lists need a sentinel of some sort to represent the "list.end()" iterator.<br>> - The way to do this efficiently and safely is to have the sentinel <br>> be some un-templated "list_node_base" class that has a next and prev<br>> pointer. The next/prev pointers point to "list_node_base", and <br>> "list_iterator<T>" downcasts to "T" on dereference.<br>> - However, ilist<T> assumes the sentinel is a full-bodied "T" and <br>> uses "T*" for the "next" pointers. In practice consumers override <br>> the sentinel traits so that the sentinel is only a "ilist_half_node"<br>> (to save space). This means that operator++ invokes UB every time <br>> you increment to the end iterator and downcast to "T*" (badness).<br>> - MachineInstrBundleIterator::operator MachineInstr*() relies on the<br>> member of ilist_iterator<MachineInstr> being an already-downcast <br>> MachineInstr*, indirectly relying on the UB in operator++. I can't <br>> safely refactor ilist/ilist_node/etc. until code stops (indirectly) <br>> relying on this UB.<br>> <br>> What's `&*I` all about?<br>> - An iterator<T> does not necessarily point at a valid T.<br>> - It could just be a sentinel object that T inherits from (in the <br>> case of lists), or an unallocated memory location (in the case of arrays).<br>> - If you really want a T*, you should dereference iterator<T> and <br>> then take the address.<br>> - Everyone (?) knows that it's undefined behaviour to dereference an<br>> iterator<T> that is not pointing at a valid T. `&*I` makes it clear<br>> at the call site that a pre-condition to getting a `T*` is for `I` <br>> to be a valid iterator.<br>> - I will be making the implicit operator illegal promptly when <br>> there's no code using it. Just waiting on a few backends.<br>> - Your code will be nicer to read -- particularly after my changes <br>> -- if you use references (T&) instead of pointers (T*) whenever <br>> variables/parameters/etc. cannot be nullptr. In many cases I've <br>> updated variables/parameters/etc. in this way, but sometimes I took <br>> the easy way out of just saying `&*I` (and sometimes that's the best<br>> option). I encourage following up with more reference-based logic!<br></tt><br><tt>Thanks for the explanation, that does make sense. So once the implicit</tt><br><tt>operator is made illegal, the compiler should catch any attempt to use</tt><br><tt>it at compile time, and we cannot accidentally reintroduce problematic</tt><br><tt>code in the future, right?</tt><br>
</p></div></blockquote>Exactly. </body></html>