<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 14, 2014 at 6:09 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div class="im"><br><div><div>On Feb 14, 2014, at 8:19 , David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Feb 14, 2014 at 1:31 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Ted, Jordan, Anna: ping.<div><br></div><div>This is really important for us - we love the analyzer, but without this patch, we basically cannot use it (apart from maintaining our own private fork, which we would rather not do, if at all possible). Every time somebody follows a code path that leads to an incorrect handling of temporary destructors, we loose a bunch of engineering time - might we be able to get this patch re-applied under a flag?</div>

</div></blockquote><div><br></div><div>I haven't at all looked at the actual patch or the false positives, but a few questions come to mind:<br><br>My understanding is that this helps fix C++ quality, but regresses some ObjC cases, yes?<br>

<br>If so, why don't we only do this when compiling C++ (and, maybe, ObjC++ - I assume it helps more than it hurts there) - in that way we wouldn't regress ObjC quality and wouldn't need a flag?<br><br>What are the specific ObjC regressions? Are they a matter of luck in the current design, or intentional but the design that works for them is inflexible to this enhancement? (this is a difficult question to explain - but I'm kind of drawing an analogy from the LLVM optimization pipeline - sometimes improving one optimization exposes flaws in another, through no fault of the improvement. We still care about the overall regression and work diligently to fix or revert, etc)</div>

<div> </div></div></div></div></blockquote><br></div></div><div>It isn't just Objective-C code. Pavel's patch revealed a hole in liveness tracking that meant impossible paths were getting reported, but only in very specific circumstances involving nested logical expressions. This can happen in any language mode. None of us have put in the time to do more than triage this and talk about <i>possible</i> solutions, which were in my original e-mail to Daniel. (The liveness issue is described in <a href="http://llvm.org/bugs/show_bug.cgi?id=18159" target="_blank">PR18159</a>, and the original ||/&& discussion is <a href="http://llvm.org/bugs/show_bug.cgi?id=11645" target="_blank">PR11645</a>. It's entirely possible to come up with answers that <i>don't</i> fix the liveness issue, too.)</div>
<div><br></div><div>If temporary destructor false positives are bad, how about a path that explicitly says "if (!x)" and then "if (x)"? That's just not a trade worth making.</div></div></blockquote>
<div><br></div><div>So if I understand you correctly, you're saying that a path that says "if (!x)" and then "if (x)" will be handled incorrectly with Pavel's patch?</div><div>My question about the option of putting it in under a flag / getting a better idea about the type of regressions it causes comes from the observation that when we included Pavel's patch the false positives went drastically down (at least on our code bases).</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div></div></div></div>