[cfe-commits] r140232 - /cfe/trunk/lib/Sema/SemaExpr.cpp
David Blaikie
dblaikie at gmail.com
Wed Sep 21 13:32:34 PDT 2011
>
> I don't think that's the correct solution for many of these cases. There
> are at least two patterns here:
>
> foo bar(...) {
> switch (x) {
> default: assert(0 && ...);
> case ...:
> case ...:
> ...
> }
>
> // handle stuff
> return my_foo;
> }
>
> and
>
> foo bar(...) {
> switch (x) {
> default: assert(0 && ...);
> case ...: return ...;
> case ...: return ...;
> ...
> }
> }
>
Yes, there are some of those - though they're actually the minority of cases
in the patch I've sent out.
> The former should probably stay assert(0 ...). It has some ability to
> handle that case in opt builds anyways. There is no control flow to prune
> out.
>
I generally disagree with this kind of "assert with fallback" behavior -
it's untested code. An assert is there because we think this is unreachable.
If it's reachable then we should have an error handling case & test those
failure cases. Otherwise we're just off the map anyway. It's not clear that
in all such cases the return after the switch is meaningful/correct/useful
in the situation where the default case would've been executed.
But I admit this is a common source of debate about assert usage.
> The latter should probably not have a default *at all*, have a covering set
> of cases, and have llvm_unreachable *outside the switch*, before the '}', to
> prevent the missing return from causing a warning.
>
Sure, I'm happy to apply this transformation in such cases, if any.
> These are two distinct patterns, and in neither case is blindly replacing
> assert(0 && ...) the correct transformation. I think each fix needs to be
> made mindful of the code context it occurs in, and what the reasonable
> behavior is there.
>
Sure - I'm happy to discuss & apply the variations/better approaches.
- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110921/a0964973/attachment.html>
More information about the cfe-commits
mailing list