dcoughlin added inline comments.
================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:321
@@ +320,3 @@
+ // sink, we assume that a client requesting a transition to a state that is
+ // the same as the predecessor state has made a mistake. We return the
+ // predecessor and rather than cache out.
----------------
zaks.anna wrote:
> dcoughlin wrote:
> > jordan_rose wrote:
> > > What does "has made a mistake" mean? What is the mistake and how will
> > > they discover it?
> > The mistake this guard protects against is adding a transition from a state
> > to itself, which would normally cause a cache out. My understanding is that
> > the whole point of this guard is to silently swallow that mistake. I found
> > that surprising, which is why I added the comment.
> Should we add an assert here? I wonder if/how much it will get triggered.
I tried adding an assert to the inverse of the 'if' condition here. The
DereferenceChecker, CallAndMessageChecker, and DynamicTypePropagation all
trigger it. Added a note about this in a comment.
================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:322
@@ -290,1 +321,3 @@
+ // the same as the predecessor state has made a mistake. We return the
+ // predecessor and rather than cache out.
if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))
----------------
xazax.hun wrote:
> As a slightly related note: is it documented anywhere what "cache out" means?
> Maybe it would be great to refer to that document or write it if it is not
> written yet.
I've defined "cache out" in the comment.
================
Comment at: lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp:53
@@ -52,3 +52,3 @@
- if (ExplodedNode *N = C.addTransition()) {
+ if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
if (!BT)
----------------
dcoughlin wrote:
> zaks.anna wrote:
> > jordan_rose wrote:
> > > zaks.anna wrote:
> > > > Can this ever fail? In some cases we just assume it won't in others we
> > > > tests..
> > > >
> > > > Maybe it only fails when we cache out?
> > > It does fail when we cache out, and I think we can still cache out if
> > > Pred has a different tag the second time around.
> > There some instances in this patch where we do not check if the returned
> > node is null. We should be consistent.
> Ok, I'll go through and add checks where they are missing.
There were 9 locations where checks for null error generated error nodes were
missing. I added them.
http://reviews.llvm.org/D12780
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
