[cfe-commits] r140232 - /cfe/trunk/lib/Sema/SemaExpr.cpp

[David Blaikie]

>
> 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>