[cfe-dev] [analyzer] Thoughts on Temporary Destructors

[Manuel Klimek]

On Fri, Feb 14, 2014 at 6:09 PM, Jordan Rose <jordan_rose at apple.com> wrote:

>
> On Feb 14, 2014, at 8:19 , David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
>
> On Fri, Feb 14, 2014 at 1:31 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> Ted, Jordan, Anna: ping.
>>
>> 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?
>>
>
> I haven't at all looked at the actual patch or the false positives, but a
> few questions come to mind:
>
> My understanding is that this helps fix C++ quality, but regresses some
> ObjC cases, yes?
>
> 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?
>
> 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)
>
>
>
> 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 *possible* solutions, which were in my
> original e-mail to Daniel. (The liveness issue is described in PR18159<http://llvm.org/bugs/show_bug.cgi?id=18159>,
> and the original ||/&& discussion is PR11645<http://llvm.org/bugs/show_bug.cgi?id=11645>.
> It's entirely possible to come up with answers that *don't* fix the
> liveness issue, too.)
>
> 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.
>

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?
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).

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140214/1d7ad356/attachment.html>