[cfe-commits] r117220 - in /cfe/trunk: lib/Analysis/CFG.cpp test/Sema/warn-unreachable.c

[Marcin Świderski]

2010/10/26 Ted Kremenek <kremenek at apple.com>

>
> On Oct 24, 2010, at 1:21 AM, Marcin Swiderski wrote:
>
> Modified: cfe/trunk/lib/Analysis/CFG.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=117220&r1=117219&r2=117220&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
> +++ cfe/trunk/lib/Analysis/CFG.cpp Sun Oct 24 03:21:40 2010
> @@ -912,15 +912,17 @@
>       AppendStmt(Block, B, asc);
>     }
>
> -    // If visiting RHS causes us to finish 'Block' and the LHS doesn't
> -    // create a new block, then we should return RBlock.  Otherwise
> -    // we'll incorrectly return NULL.
> -    CFGBlock *RBlock = Visit(B->getRHS());
> -    CFGBlock *LBlock = Visit(B->getLHS(),
> AddStmtChoice::AsLValueNotAlwaysAdd);
> -    return LBlock ? LBlock : RBlock;
> +    Visit(B->getLHS(), AddStmtChoice::AsLValueNotAlwaysAdd);
> +    return Visit(B->getRHS());
>   }
>
>
> Hi Marcin,
>
> Shouldn't we invert the check that we had before?  Visit(B->getRHS()) isn't
> guaranteed to return a non-NULL block if visiting the LHS resulted in
> finishing up a CFGBlock.  That was the purpose of the previous logic (when
> the LHS was visited first).
>
> Ted
>

Because:
1. removing the check after swaping the order did not crash tests,
2. VisitChildren() is implemented this way,
I assumed that this is the right way to go. If I'm mistaken then shouldn't
we add such check in other places with similar code e.g. VisitChildren()?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101026/c56ca9b2/attachment.html>