sconstab added inline comments.
================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:73 + // The end of this Node's edges is the beginning of the next node's edges. + const Edge *edges_end() const { return (this + 1)->Edges; } + ArrayRef<Edge> edges() const { ---------------- mattdr wrote: > sconstab wrote: > > mattdr wrote: > > > Seems like you also want to add a comment here that we know we will never > > > be asked for `edges_end` for the last stored node -- that is, we know > > > that `this + 1` always refers to a valid Node (which is presumably a > > > dummy/sentinel) > > Not sure I agree. I cannot think of a conventional use of this interface > > that would perform an operation on the sentinel. > > ``` > > G->nodes_end().edges_end(); // invalid use of any end iterator > > SomeNode.edges_end(); // invalid if SomeNode == G->nodes_end() > > ``` > > That is, the way that we "know" that we will never be asked for > > `edges_end()` for the last stored node is that the ask itself would already > > violate C++ conventions. > I believe any operation on the last `Node` in the array will end up accessing > the sentinel: > > ``` > Node* LastNode = G->nodes_begin() + (G->nodes_size() - 1); // valid > reference to the last node > LastNode->edges_end(); // uses `this+1`, accessing the sentinel value in the > Nodes array > ``` > `G->nodes_size()` will return the size without the sentinel node, so your example should actually operate on the last data node. Right? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75936/new/ https://reviews.llvm.org/D75936 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits