On Thu, Aug 7, 2014 at 9:16 PM, David Blaikie <[email protected]> wrote:
> On Thu, Aug 7, 2014 at 11:44 AM, Manuel Klimek <[email protected]> wrote: > > Author: klimek > > Date: Thu Aug 7 13:44:19 2014 > > New Revision: 215128 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=215128&view=rev > > Log: > > Mark successors as reachable/unreachable instead of changing the CFG. > > > > As suggested by Ted, this makes a few warnings less aggressive. > > > > Modified: > > cfe/trunk/lib/Analysis/CFG.cpp > > cfe/trunk/test/SemaCXX/return-noreturn.cpp > > > > Modified: cfe/trunk/lib/Analysis/CFG.cpp > > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=215128&r1=215127&r2=215128&view=diff > > > ============================================================================== > > --- cfe/trunk/lib/Analysis/CFG.cpp (original) > > +++ cfe/trunk/lib/Analysis/CFG.cpp Thu Aug 7 13:44:19 2014 > > @@ -479,6 +479,7 @@ private: > > AbstractConditionalOperator *E, bool BindToTemporary, > > TempDtorContext &Context); > > void InsertTempDtorDecisionBlock(const TempDtorContext &Context, > > + TryResult ConditionVal, > > CFGBlock *FalseSucc = nullptr); > > > > // NYS == Not Yet Supported > > @@ -3631,24 +3632,16 @@ CFGBlock *CFGBuilder::VisitBinaryOperato > > BinaryOperator *E, TempDtorContext &Context) { > > if (E->isLogicalOp()) { > > VisitForTemporaryDtors(E->getLHS(), false, Context); > > - TryResult LHSVal = tryEvaluateBool(E->getLHS()); > > - bool RHSNotExecuted = (E->getOpcode() == BO_LAnd && > LHSVal.isFalse()) || > > - (E->getOpcode() == BO_LOr && LHSVal.isTrue()); > > - if (RHSNotExecuted) { > > - return Block; > > - } > > - > > - // If the LHS is known, and the RHS is not executed, we returned > above. > > - // Thus, once we arrive here, and the LHS is known, we also know > that the > > - // RHS was executed and can execute the RHS unconditionally (that > is, we > > - // don't insert a decision block). > > - if (LHSVal.isKnown()) { > > - VisitForTemporaryDtors(E->getRHS(), false, Context); > > - } else { > > - TempDtorContext RHSContext(/*IsConditional=*/true); > > - VisitForTemporaryDtors(E->getRHS(), false, RHSContext); > > - InsertTempDtorDecisionBlock(RHSContext); > > - } > > + TryResult RHSExecuted = tryEvaluateBool(E->getLHS()); > > + if (RHSExecuted.isKnown() && E->getOpcode() == BO_LOr) > > + RHSExecuted.negate(); > > + > > + // We do not know at CFG-construction time whether the > right-hand-side was > > + // executed, thus we add a branch node that depends on the temporary > > + // constructor call. > > + TempDtorContext RHSContext(/*IsConditional=*/true); > > + VisitForTemporaryDtors(E->getRHS(), false, RHSContext); > > + InsertTempDtorDecisionBlock(RHSContext, RHSExecuted); > > > > return Block; > > } > > @@ -3705,6 +3698,7 @@ CFGBlock *CFGBuilder::VisitCXXBindTempor > > } > > > > void CFGBuilder::InsertTempDtorDecisionBlock(const TempDtorContext > &Context, > > + TryResult ConditionVal, > > CFGBlock *FalseSucc) { > > if (!Context.TerminatorExpr) { > > // If no temporary was found, we do not need to insert a decision > point. > > @@ -3713,8 +3707,9 @@ void CFGBuilder::InsertTempDtorDecisionB > > assert(Context.TerminatorExpr); > > CFGBlock *Decision = createBlock(false); > > Decision->setTerminator(CFGTerminator(Context.TerminatorExpr, true)); > > - addSuccessor(Decision, Block); > > - addSuccessor(Decision, FalseSucc ? FalseSucc : Context.Succ); > > + addSuccessor(Decision, Block, !ConditionVal.isFalse()); > > + addSuccessor(Decision, FalseSucc ? FalseSucc : Context.Succ, > > + !ConditionVal.isTrue()); > > Block = Decision; > > } > > > > @@ -3725,16 +3720,8 @@ CFGBlock *CFGBuilder::VisitConditionalOp > > CFGBlock *ConditionBlock = Block; > > CFGBlock *ConditionSucc = Succ; > > TryResult ConditionVal = tryEvaluateBool(E->getCond()); > > - > > - if (ConditionVal.isKnown()) { > > - if (ConditionVal.isTrue()) { > > - VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, > Context); > > - } else { > > - assert(ConditionVal.isFalse()); > > - VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, > Context); > > - } > > - return Block; > > - } > > + TryResult NegatedVal = ConditionVal; > > + if (NegatedVal.isKnown()) NegatedVal.negate(); > > > > TempDtorContext TrueContext(/*IsConditional=*/true); > > VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary, > TrueContext); > > @@ -3746,12 +3733,12 @@ CFGBlock *CFGBuilder::VisitConditionalOp > > VisitForTemporaryDtors(E->getFalseExpr(), BindToTemporary, > FalseContext); > > > > if (TrueContext.TerminatorExpr && FalseContext.TerminatorExpr) { > > - InsertTempDtorDecisionBlock(FalseContext, TrueBlock); > > + InsertTempDtorDecisionBlock(FalseContext, NegatedVal, TrueBlock); > > } else if (TrueContext.TerminatorExpr) { > > Block = TrueBlock; > > - InsertTempDtorDecisionBlock(TrueContext); > > + InsertTempDtorDecisionBlock(TrueContext, ConditionVal); > > } else { > > - InsertTempDtorDecisionBlock(FalseContext); > > + InsertTempDtorDecisionBlock(FalseContext, NegatedVal); > > } > > return Block; > > } > > > > Modified: cfe/trunk/test/SemaCXX/return-noreturn.cpp > > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/return-noreturn.cpp?rev=215128&r1=215127&r2=215128&view=diff > > > ============================================================================== > > --- cfe/trunk/test/SemaCXX/return-noreturn.cpp (original) > > +++ cfe/trunk/test/SemaCXX/return-noreturn.cpp Thu Aug 7 13:44:19 2014 > > @@ -185,11 +185,11 @@ int testTernaryConditionalNoreturnFalseB > > > > int testConditionallyExecutedComplexTernaryTrueBranch(bool value) { > > value || (true ? NoReturn() : true); > > -} // expected-warning {{control may reach end of non-void function}} > > +} > > I'm a bit confused as to why /this/ warning would go away when you add > the optimstic/pessimistic edges. This warning should still fire, as, > when 'value' is false, control does reach the end of a non-void > function. > Yea, I think the problem is that when we annotate a branch with reachability information, we have to always add a CFG branch at the next higher level branch point until we hit an unannotated branch. I'll add that tomorrow. > > The warnings I would expect to not fire with the kind of change Ted > was suggesting (and I don't know if this is a pre-existing problem, an > issue with this patch, or an issue with my understanding) would be > things like -Wunreachable-code, probably in cases like: > > true ? NoReturn() : true; > func(); // -Wunreachable-code should fire here > > ARCH_CONST ? NoReturn() : true; > func(); // don't warn that this is unreachable, because ARCH_CONST > could vary by build > > Or something like that - I forget which particular heuristics Ted put > in to suppress build-varying constants. > > > int testConditionallyExecutedComplexTernaryFalseBranch(bool value) { > > value || (false ? true : NoReturn()); > > -} // expected-warning {{control may reach end of non-void function}} > > +} > > > > int testStaticallyExecutedLogicalOrBranch() { > > false || NoReturn(); > > @@ -209,7 +209,7 @@ int testStaticallySkppedLogicalAndBranch > > > > int testConditionallyExecutedComplexLogicalBranch(bool value) { > > value || (true && NoReturn()); > > -} // expected-warning {{control may reach end of non-void function}} > > +} > > > > #if __cplusplus >= 201103L > > namespace LambdaVsTemporaryDtor { > > > > > > _______________________________________________ > > cfe-commits mailing list > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
