Szelethus added inline comments.
================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:230
+
+ virtual void releaseMemory() {
+ PostDomTree.releaseMemory();
----------------
kuhar wrote:
> If the virtual function is introduced by ManagesAnalysis, isn't it better to
> use override here?
I admit to have copy-pasted this from the class above, and, well, it isn't
introduced by `ManagedAnalysis`. Which begs the question why it was made
virtual at all.
Nice catch!
================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:235
+ // Lazily retrieves the set of control dependencies to \p A.
+ const CFGBlockVector &getControlDependencies(CFGBlock *A) {
+ auto It = ControlDepenencyMap.find(A);
----------------
kuhar wrote:
> Can it be const?
IDFCalculator takes it's arguments by a non-const pointer. I guess I could fix
that too on LLVM's side eventually. The method itself retrieves control
dependencies lazily, and might modify the state of this class.
================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:252
+ /// Whether \p A is control dependent on \p B.
+ bool isControlDependent(CFGBlock *A, CFGBlock *B) {
+ return llvm::is_contained(getControlDependencies(A), B);
----------------
kuhar wrote:
> Can it be const?
Same.
================
Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+ // Dumps immediate control dependencies for each block.
+ void dump() {
+ CFG *cfg = PostDomTree.getCFG();
----------------
NoQ wrote:
> kuhar wrote:
> > kuhar wrote:
> > > Can `dump` be const?
> > In LLVM, `dump`s are usually annotated with the `LLVM_DUMP_METHOD`
> > attribute and not compiled in release builds. Is the convention different
> > in the static analyzer?
> `LLVM_DUMP_METHOD` is great. Hiding dump methods under `#ifndef NDEBUG` is
> something i've seen very rarely. It's fairly annoying to me that exploded
> graph dumps are unavailable in release builds, but apart from that i don't
> have any immediate opinion, so this sounds like a global LLVM policy that
> we're historically not paying much attention to, but i don't mind complying.
Cant be const for the same reasons.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62619/new/
https://reviews.llvm.org/D62619
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
