<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 3, 2013, at 3:21 , Richard <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On 2 Oct 2013, at 03:23, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 1, 2013, at 9:40 , Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>[+cfe-commits, which I didn't really mean to remove]</div><br><div><div>On Sep 27, 2013, at 1:01 , Richard <<a href="mailto:tarka.t.otter@gmail.com">tarka.t.otter@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div>On 25 Sep 2013, at 18:44, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br><div>Yes, the SVal was an UnknownVal, that was it. I have fixed the dtor.cpp test by forcing a transition for StNonNull at the end of checkPreStmt with:</div><div><br></div><div><div style="margin: 0px; font-size: 14px; font-family: Monaco; ">  C.addTransition(StNonNull, <span style="color: #bb2ca2">this</span>);</div></div><div><br></div><div>Unfortunately the other test failure remains, method-call-path-notes.cpp. This one is a bit trickier, checkPreCall is now being called and the null state is triggering a bug report. The problem is the generateSink() method in emitBadCall() is returning null, so the bug is never actually reported.</div><div><br></div><div>I guess the previously generated sink is causing the new sink to not be generated, but I do not know how to work around this. The generated graph is also a bit odd, I have attached it in case it is any help. Any suggestions here?</div></div></blockquote><div><br></div><div>Hm, I wonder if <i>this</i> is from the missing return at the end of the null check clause in checkPreStmt. Transitioning to StNonNull when we <i>know</i> the value is null is just wrong. I'll commit that back soon, but meanwhile can you try it locally?</div><div><br></div><div>(That probably also means you don't have to add the ", this" to the final addTransition there.)</div></div></div></blockquote><br></div><div>Committed in r191805.</div><div><br></div><div>Jordan</div><br></div></blockquote></div><br><div>Unfortunately this does not fix either issue, as in both cases we have an UnknownVal for the SVal, so we never hit the early return. </div><div><br></div><div>I am wondering if C++ member expressions should have special handling in the CallAndMessageChecker. Is it possible for a C++ member function to be null? I'm afraid my knowledge of C++ is insufficient to offer anything more constructive at this point, but doing an early return from CallAndMessageChecker::checkPreStmt for <span style="font-family: Monaco; font-size: 14px; ">CXXMemberCallExpr </span>fixes the bug, and does not cause any new ones to materialise.</div></div></blockquote><br></div><div>Member pointers can most certainly be null, but we don't handle it yet:</div><div><br></div><div><div><font face="Menlo"><span style="font-size: 11px;">struct Foo {</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><span class="Apple-tab-span" style="white-space:pre">  </span>int bar();</span></font></div><div><font face="Menlo"><span style="font-size: 11px;">};</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><br></span></font></div><div><font face="Menlo"><span style="font-size: 11px;">void test(Foo *obj) {</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><span class="Apple-tab-span" style="white-space:pre">     </span>int (Foo::*mp)() = 0;</span></font></div><div><font face="Menlo"><span style="font-size: 11px;"><span class="Apple-tab-span" style="white-space:pre">    </span>(obj->*mp)(); // should warn but don't</span></font></div><div><font face="Menlo"><span style="font-size: 11px;">}</span></font></div><div><br></div><div>Instead, let's think about why that would fix the problem. We shouldn't be generating a sink node anymore in the non-null case, and either we changed the state or we didn't. With the change to CheckerContext::addTransition, though, checkPreStmt generates a new node tagged with the checker, but with the same state as before. Then when we get to checkPreCall, we try to do the same thing again, and <i>there</i> we cache out.</div><div><br></div><div>I think what this means is that rather than completely removing the check in CheckerContext::addTransition, we should either check if the predecessor node is tagged, or just document that if nothing changed, you don't get a new node unless you explicitly pass the checker as a tag. How many places would need to change if you had to do that?</div><div><br></div><div>Jordan</div></div><br></body></html>