Ok, fixed this in r215128 ... That indeed makes the code significantly simpler.
On Thu, Aug 7, 2014 at 7:15 PM, Manuel Klimek <[email protected]> wrote: > On Thu, Aug 7, 2014 at 7:14 PM, Ted Kremenek <[email protected]> wrote: > >> >> On Aug 7, 2014, at 10:10 AM, Manuel Klimek <[email protected]> wrote: >> >> On Thu, Aug 7, 2014 at 7:06 PM, Ted Kremenek <[email protected]> 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 :) >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
