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

Jakub Kuderski via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue May 28 14:17:33 PDT 2019

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
-  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?

  rC Clang



More information about the cfe-commits mailing list