[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

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