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

[Manuel Klimek]

Ok, fixed this in r215128 ... That indeed makes the code significantly
simpler.


On Thu, Aug 7, 2014 at 7:15 PM, Manuel Klimek <klimek at google.com> wrote:

> 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/5371b5bc/attachment.html>