arsenm added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:598
} else {
- assert(false && "Unhandled type in array initializer initlist");
+ llvm_unreachable("Unhandled type in array initializer initlist");
}
----------------
aaron.ballman wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > The rest of the ones here are somewhat interesting in that the
> > > interpreter is an experiment under active development and is known to be
> > > incomplete. In all of these cases, I think the switch to unreachable is
> > > flat-out wrong -- these asserts serve explicitly to find unimplemented
> > > cases when we hit them.
> > & I don't see why unreachable is any different a statement than
> > assert(false) in these cases... - it's the same statement of intent. "if
> > this is reached you've found a bug" (in this case, a missing feature)
> >
> > But I'd be sort of OK changing all these to report_fatal_error. But, again,
> > I think the assert(false) -> unreachable is a valid transformation and
> > doesn't make anything worse than it already is, but improves the code by
> > being more consistent and removing this confusion that there might be
> > something different about assert(false) when, I believe, there isn't.
> > & I don't see why unreachable is any different a statement than
> > assert(false) in these cases... - it's the same statement of intent. "if
> > this is reached you've found a bug" (in this case, a missing feature)
>
> You are asserting it's the same statement of intent and I keep pointing out
> that people use the different constructs in practice because they're
> different statements of intent. I don't know how to resolve this difference
> of opinion, but I can say as someone doing code review in this area recently
> that your interpretation is wrong according to what we were after with this
> code.
>
> I'd be fine changing it to `report_fatal_error` instead of `assert(false)`;
> I'd be strongly opposed to switching to `llvm_unreachable`.
I use llvm_unreachable as a nicer to use assert in if/else chains like this. I
also see no difference in the intent between assert and unreachable; assert(0
&& "message") is just uglier.
report_fatal_error is for something a user could plausibly run into but also
isn't worth wiring into a proper error diagnostic (which happens a lot in
codegen)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135551/new/
https://reviews.llvm.org/D135551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
