[PATCH] D48383: [Dominators] Add the DomTreeUpdater class

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

Looks almost ready to me. Found just a couple of nits.



================
Comment at: include/llvm/IR/DomTreeUpdater.h:58
+
+  /// Returns false if there isn't ant pending update for the holding trees
+  /// regardless of whether there is BasicBlock awaiting deletion.
----------------
I'm not sure that `holding trees` is grammatically correct in this place and all the other ones.
Maybe `held trees` or just `available trees`.
Perhaps @brzycki could help here.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:130
+  /// Delete DelBB. DelBB will be removed from its Parent and
+  /// be erased from holding trees if it exists. Then the callback will
+  /// be called. Finally, DelBB will be deleted.
----------------
please remove `be`


================
Comment at: include/llvm/IR/DomTreeUpdater.h:157
+
+  /// Flush all updates and deleted BBs.
+  void flush();
----------------
Can you add one extra sentence saying what it actually means to flush?


================
Comment at: lib/IR/DomTreeUpdater.cpp:36
+  const bool HasEdge =
+      std::any_of(succ_begin(From), succ_end(From),
+                  [To](const BasicBlock *B) { return B == To; });
----------------
kuhar wrote:
> Why not llvm::find?
Missed comment?


================
Comment at: lib/IR/DomTreeUpdater.cpp:248
+  while (!DelBB->empty()) {
+    Instruction &I = DelBB->back();
+    // Replace used instructions with an arbitrary value (undef).
----------------
Can a BB have 0 instructions?


https://reviews.llvm.org/D48383