[PATCH] D56632: [analyzer] Track region liveness only through base regions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:390
 
-  // FIXME: Remove when we migrate over to just using ValueManager.
   SymbolManager &getSymbolManager() { return SymMgr; }
----------------
baloghadamsoftware wrote:
> Is this comment intentionally deleted?
Yeah, i don't think anybody remembers what was that about and there doesn't seem to be an immediate need in something like that.

Hmm, why did i delete it as part of that revision? I guess because i was moving these helper methods around. Let me bring them back because this place is actually better, now that i think about it.

Also i wonder if anybody ever uses this const getter two lines below (or even passes `ExprEngine` by const reference anywhere). Hmm, seems to compile without it.


================
Comment at: test/Analysis/symbol-reaper.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+
----------------
Szelethus wrote:
> Core intentionally left out?
Thx ^.^ No, just still have this habit since before it was decided to make it mandatory >.<


================
Comment at: test/Analysis/symbol-reaper.cpp:13
+  void foo() {
+    // Ugh, maybe just let clang_analyzer_eval() work within callees already?
+    // The glob variable shouldn't keep our symbol alive because
----------------
a_sidorin wrote:
> //FIXME?
Yeah, i guess it's a more polite way of expressing it :)


================
Comment at: test/Analysis/symbol-reaper.cpp:31
+  clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+                             // expected-warning at -1{{SYMBOL DEAD}}
+}
----------------
Szelethus wrote:
> N00b question: What does `SYMBOL DEAD` mean here exactly?
It's a warning produced by `clang_analyzer_warnOnDeadSymbol(a.x)` when the value that was in `a.x` (that was there when that function was called) dies. This is an `ExprInspection` utility that was created in order to test `SymbolReaper` more directly. See `symbol-reaper.c` for more such tests.


================
Comment at: unittests/StaticAnalyzer/SymbolReaperTest.cpp:52
+    assert(Matches.size() == 1 && "Ambiguous name!");
+    for (BoundNodes &M : Matches)
+      return M.getNodeAs<T>("d");
----------------
a_sidorin wrote:
> It looks like `selectFirst` helper is what you need here.
Wow, this one's handy.


================
Comment at: unittests/StaticAnalyzer/SymbolReaperTest.cpp:97
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+    for (auto D: DG)
+      performTest(D);
----------------
a_sidorin wrote:
> Nit: `const auto *D : DG`
Fxd^^


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

https://reviews.llvm.org/D56632