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

[Chandler Carruth]

On Wed, Sep 21, 2011 at 12:42 PM, David Blaikie <dblaikie at gmail.com> wrote:

> I'm about to send out a patch to that effect. If someone (Doug, John, etc)
> wants to just say "yes" when they see that, I'll check it in.


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 ...;
    ...
  }
}

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.

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110921/61ed020d/attachment.html>