mattdr added a comment. Some more comments. FWIW, I'm doing rounds of review as I can in some evening quiet or during my son's nap. This is a huge change and it's really hard to get any part of it into my head at once in a reasonable amount of time.
================ 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 { ---------------- 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 ``` ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:79 + +protected: + ImmutableGraph(std::unique_ptr<Node[]> Nodes, std::unique_ptr<Edge[]> Edges, ---------------- sconstab wrote: > mattdr wrote: > > Why "protected" rather than "private"? Usually seeing "protected" makes me > > think subclassing is expected, but that doesn't seem to be the case here. > The `MachineGadgetGraph` class actually does subclass `ImmutableGraph` to add > some contextual information. I did not want the constructors for > `ImmutableGraph` to be public, because the intent is to use the builder. So > `protected` seemed like the best option. Ah, I missed that. I searched through the file for `public ImmutableGraph` and didn't find it because `MachineGadgetGraph` uses the default inheritance specifier. ================ Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:83-85 +#define ARG_NODE nullptr +#define GADGET_EDGE ((int)(-1)) +#define WEIGHT(EdgeValue) ((double)(2 * (EdgeValue) + 1)) ---------------- Please replace these with constants or functions. ================ Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:113 + NumFences(NumFences), NumGadgets(NumGadgets) {} + MachineFunction &getMF() { // FIXME: This function should be cleaner + for (const Node &N : nodes()) ---------------- sconstab wrote: > mattdr wrote: > > Cleaner how? > Maybe by keeping a member reference to the associated `MachineFunction`? Let's put that in the comment instead. 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