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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
