[PATCH] D88758: [DomTree] findNearestCommonDominator: assert the nodes are in tree

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

Does this break any existing code?



================
Comment at: llvm/include/llvm/Support/GenericDomTree.h:483
     DomTreeNodeBase<NodeT> *NodeB = getNode(B);
-
-    if (!NodeA || !NodeB) return nullptr;
+    assert(NodeA && NodeB);
 
----------------
kuhar wrote:
> I both `NodeA` and `NodeB` have to be non-null before the loop, and we unconditionally dereference `NodeA` at the return statement, then one of the three must be off: (1) the assertion, (2) the loop condition, (3) the return.
> 
Maybe assert each node separately and assertion message explaining what this means?


================
Comment at: llvm/include/llvm/Support/GenericDomTree.h:483-493
+    assert(NodeA && NodeB);
 
     // Use level information to go up the tree until the levels match. Then
     // continue going up til we arrive at the same node.
     while (NodeA && NodeA != NodeB) {
       if (NodeA->getLevel() < NodeB->getLevel()) std::swap(NodeA, NodeB);
 
----------------
I both `NodeA` and `NodeB` have to be non-null before the loop, and we unconditionally dereference `NodeA` at the return statement, then one of the three must be off: (1) the assertion, (2) the loop condition, (3) the return.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88758/new/

https://reviews.llvm.org/D88758