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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
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);
 
----------------
kuhar wrote:
> 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?
Good catch. I have deleted the redundant check.

`check-llvm check-clang` passed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88758