[PATCH] D62551: [analyzer][Dominator] Add post dominator tree builder for the CFG + a debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:39
 
 /// Concrete subclass of DominatorTreeBase for Clang
 /// This class implements the dominators tree functionality given a Clang CFG.
----------------
Seems outdated?


================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:47
 public:
-  llvm::DomTreeBase<CFGBlock> *DT;
+  using DomTreeBase = llvm::DominatorTreeBase<CFGBlock, IsPostDom>;
 
----------------
nit: I think this can name can be somewhat confusing as it's not a base class of CFGDomTree.


================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:49
 
-  DominatorTree() {
-    DT = new llvm::DomTreeBase<CFGBlock>();
+  DomTreeBase *DT;
+
----------------
Why not  have it as a value member? Or a unique_ptr, if pointer semantics are really desired.


================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:111
+      assert(
+          (IDom && !IDom->getBlock() && *I == &(*I)->getParent()->getExit() &&
+           IsPostDom) ||
----------------
Assertions with multiple conditions conjoined are difficult to debug -- perhaps it'd be better to split them and assign to separate booleans for each condition, with descriptive names? This code can be hidden behind an ifdef for debug. 


================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:174
 
+using DominatorTree = CFGDominatorTree</*IsPostDom*/ false>;
+using PostDominatorTree = CFGDominatorTree</*IsPostDom*/ true>;
----------------
nit: How about having the parent class template called CFGDominatorTreeImpl and these two as CFGDomTree and CFGPostdomTree?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62551