[PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Devin Coughlin via cfe-commits]

dcoughlin marked 3 inline comments as done.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:229
@@ -228,2 +228,3 @@
+  /// checkers should use generateErrorNode() instead.
   ExplodedNode *generateSink(ProgramStateRef State = nullptr,
                              ExplodedNode *Pred = nullptr,
----------------
zaks.anna wrote:
> Most likely there are not much uses of this left and to avoid confusion we could require State and Pred inputs. What do you think?
There are 7 uses left. Requiring State and Pred seems like the right thing to me. I will change it.

================
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.
----------------
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.

================
Comment at: lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp:53
@@ -52,3 +52,3 @@
 
-  if (ExplodedNode *N = C.addTransition()) {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     if (!BT)
----------------
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.


http://reviews.llvm.org/D12780