<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 7, 2014 at 7:06 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Manuel,<br>
<br>
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:<br>
<br>
if (sizeof(foo) > 4) { /* possibly dead; -Wunreachable-code doesn't warn */ )<br>
<br>
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:<br>
<br>
addSuccessor(Block, ThenBlock, /* isReachable = */ !KnownVal.isFalse());<br>
addSuccessor(Block, ElseBlock, /* isReachable = */ !KnownVal.isTrue());<br>
<br>
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?<br>
</blockquote><div><br></div><div>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...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
Ted<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
> On Aug 7, 2014, at 7:25 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
><br>
> Author: klimek<br>
> Date: Thu Aug 7 09:25:43 2014<br>
> New Revision: 215114<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215114&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215114&view=rev</a><br>
> Log:<br>
> Only have one path in the CFG for ternaries if the condition is known.<br>
><br>
> The return type analysis requires that the CFG is simplified when the<br>
> truth values of branches are statically known at analysis time.<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Analysis/CFG.cpp<br>
> cfe/trunk/test/SemaCXX/return-noreturn.cpp<br>
><br>
> Modified: cfe/trunk/lib/Analysis/CFG.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=215114&r1=215113&r2=215114&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=215114&r1=215113&r2=215114&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)<br>
> +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Aug 7 09:25:43 2014<br>
> @@ -3711,15 +3711,24 @@ CFGBlock *CFGBuilder::VisitConditionalOp<br>
> VisitForTemporaryDtors(E->getCond(), false, Context);<br>
> CFGBlock *ConditionBlock = Block;<br>
> CFGBlock *ConditionSucc = Succ;<br>
> + TryResult ConditionVal = tryEvaluateBool(E->getCond());<br>
><br>
> TempDtorContext TrueContext(/*IsConditional=*/true);<br>
> - VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, TrueContext);<br>
> + if (!ConditionVal.isFalse()) {<br>
> + VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, TrueContext);<br>
> + if (ConditionVal.isTrue())<br>
> + return Block;<br>
> + }<br>
> CFGBlock *TrueBlock = Block;<br>
><br>
> Block = ConditionBlock;<br>
> Succ = ConditionSucc;<br>
> TempDtorContext FalseContext(/*IsConditional=*/true);<br>
> - VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, FalseContext);<br>
> + if (!ConditionVal.isTrue()) {<br>
> + VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, FalseContext);<br>
> + if (ConditionVal.isFalse())<br>
> + return Block;<br>
> + }<br>
><br>
> if (TrueContext.TerminatorExpr && FalseContext.TerminatorExpr) {<br>
> InsertTempDtorDecisionBlock(FalseContext, TrueBlock);<br>
><br>
> Modified: cfe/trunk/test/SemaCXX/return-noreturn.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215114&r1=215113&r2=215114&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215114&r1=215113&r2=215114&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/SemaCXX/return-noreturn.cpp (original)<br>
> +++ cfe/trunk/test/SemaCXX/return-noreturn.cpp Thu Aug 7 09:25:43 2014<br>
> @@ -159,6 +159,22 @@ int testTernaryUnconditionalNoreturn() {<br>
> true ? NoReturn() : NoReturn();<br>
> }<br>
><br>
> +int testTernaryStaticallyConditionalNoretrunOnTrue() {<br>
> + true ? NoReturn() : Return();<br>
> +}<br>
> +<br>
> +int testTernaryStaticallyConditionalRetrunOnTrue() {<br>
> + true ? Return() : NoReturn();<br>
> +} // expected-warning {{control reaches end of non-void function}}<br>
> +<br>
> +int testTernaryStaticallyConditionalNoretrunOnFalse() {<br>
> + false ? Return() : NoReturn();<br>
> +}<br>
> +<br>
> +int testTernaryStaticallyConditionalRetrunOnFalse() {<br>
> + false ? NoReturn() : Return();<br>
> +} // expected-warning {{control reaches end of non-void function}}<br>
> +<br>
> int testTernaryConditionalNoreturnTrueBranch(bool value) {<br>
> value ? (NoReturn() || NoReturn()) : Return();<br>
> } // expected-warning {{control may reach end of non-void function}}<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>