[PATCH] D38427: Graph builder implementation.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

I'm having difficulty following this because you document what the GraphBuilder methods do but you never say how and I get lost before I figure that out. Maybe that could go in the GraphBuilder.cpp file header comments?



================
Comment at: tools/llvm-cfi-verify/GraphBuilder.cpp:1
+#include "GraphBuilder.h"
+
----------------
Missing header


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:49
+
+extern uint64_t SearchLengthForUndef;
+extern uint64_t SearchLengthForConditionalBranch;
----------------
Do these need to be externally visible?


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:67
+// Print the provided node to standard output.
+void printConditionalBranchNode(const ConditionalBranchNode *Node);
+void printControlFlowNode(const ControlFlowNode *Node, unsigned depth);
----------------
Should be a method of ConditionalBranchNode (also, prefer to accept a raw_ostream& instead of hardcoding outs() for print methods, dump() typically writes to dbgs() I think)


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:68
+void printConditionalBranchNode(const ConditionalBranchNode *Node);
+void printControlFlowNode(const ControlFlowNode *Node, unsigned depth);
+std::vector<uint64_t>
----------------
ditto


================
Comment at: tools/llvm-cfi-verify/GraphBuilder.h:76
+  uint64_t BaseInstructionAddress;
+  std::vector<ConditionalBranchNode *> BranchNodes;
+  std::vector<ControlFlowNode *> OrphanedNodes;
----------------
Why are these pointers?


https://reviews.llvm.org/D38427