[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks, this looks very reasonable!

I agree that the syntax pointed out by @george.karpenkov is much cleaner.



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h:30
 ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
+                                    ASTContext &ASTCtx,
                                     const LocationContext *LCtx,
----------------
You can obtain the `ASTContext` either from `PrevState->getStateManager()` or from `LCtx->getAnalysisDeclContext()`, there's no need to pass it separately.


================
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:46-48
+  explicit Callback(const LocationContext *LCtx_, MemRegionManager &MRMgr_,
+                    RegionAndSymbolInvalidationTraits &ITraits_)
+      : LCtx(LCtx_), MRMgr(MRMgr_), ITraits(ITraits_) {}
----------------
We usually just write `X(Y y): y(y) {}` and don't bother about name collisions.


================
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89
+                    new Callback(LCtx, MRMgr, ITraits));
+  Finder.matchAST(ASTCtx);
+
----------------
george.karpenkov wrote:
> ormris wrote:
> > george.karpenkov wrote:
> > > IMO using the iterator directly (e.g. like it was done in https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213) leads to a much cleaner code and saves you from having to define a callback class.
> > Hmm... I think that's a better approach. Let me see if I can get that working.
> @ormris Yeah I'm really not sure why all examples use the callback API by default.
Also, please match only the local AST, i.e. the body of the function under analysis, which can be obtained as `LCtx->getDecl()->getBody()`. There's no need to scan the whole translation unit.


================
Comment at: test/Analysis/loop-widening-preserve-reference-type.cpp:13
+  for (int i = 0; i < 10; ++i) { }
+  clang_analyzer_eval(&x == &x); // expected-warning{{TRUE}}
+}
----------------
The expression is trivially true, i don't think it's exactly the thing we want to be testing.

I'm not sure how to come up with a better test here. One good thing to test, which i'd prefer, would be `&x != 0` - which should be true because stack objects can't have address `0` and the analyzer is supposed to know that, but the symbolic pointer that would have overwritten `x` during over-aggressive widening could as well be null.

Other alternatives are to add some sort of `clang_analyzer_getMemorySpace()` and check that the variable is still on the stack (which tests more, but is also more work) or use `clang_analyzer_dump()`/`clang_analyzer_explain()` to verify the exact value (but that'd test too much as it'd also test the dump format, which is redundant).


Repository:
  rC Clang

https://reviews.llvm.org/D47044