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