r215114 - Only have one path in the CFG for ternaries if the condition is known.

[Manuel Klimek]

On Thu, Aug 7, 2014 at 7:14 PM, Ted Kremenek <kremenek at apple.com> wrote:

>
> On Aug 7, 2014, at 10:10 AM, Manuel Klimek <klimek at google.com> wrote:
>
> On Thu, Aug 7, 2014 at 7:06 PM, Ted Kremenek <kremenek at apple.com> wrote:
>
>> Hi Manuel,
>>
>> This is a good optimization, but I worry about its implications on
>> -Wunreachable-code.  We have a whole bunch of heuristics in
>> -Wunreachable-code to not warn about dead code that is really only
>> conditionally dead because of platform-specific constants.  For example:
>>
>>   if (sizeof(foo) > 4) { /* possibly dead; -Wunreachable-code doesn't
>> warn */ )
>>
>> To model this in the CFG, we actually still add the successors, but mark
>> them as being possibly unreachable.  For example, here is some code
>> elsewhere in CFG.cpp:
>>
>>   addSuccessor(Block, ThenBlock, /* isReachable = */
>> !KnownVal.isFalse());
>>   addSuccessor(Block, ElseBlock, /* isReachable = */ !KnownVal.isTrue());
>>
>> What happens is that most clients of the CFG see that the branch is dead,
>> but other clients can recover the original edge.  By just outright pruning
>> the edge we lose this information.  We want the same heuristics for dead
>> code warnings to apply to ternary operators as well.  Instead of doing an
>> early return, can you just use this modified version of addSuccessor?
>>
>
> Ok, I have a current fix for a more obvious bug and will add tests for
> platform specific constants next. Otherwise also feel free to revert in the
> meantime if it breaks something for you...
>
>
> Thanks Manuel.
>
> I don't think it is breaking anything; it will just introduce false
> positives for -Wunreachable-code that ideally we will want to plug soon.  I
> think the natural tests here would be to extend the -Wunreachable-code test
> cases with more examples of ternary operations, and how we don't warn on
> cases that truly rely on platform specific constants.  We already have
> examples of this already for 'if'.
>
> The upshot is that we want the CFG to be able to retain some of the
> information for unreachable blocks as some analyses don't want to see the
> pruning.  It's a fundamental design shift we introduced in the analyzer 4-5
> months ago.  Before we just hard pruned branches.
>

I actually think I have added all the tests already, and just have to flip
them to not warn :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140807/82f3472c/attachment.html>