aaronpuchert added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:12367
+ case Type::Class:
\
+ llvm_unreachable("Unexpected " Kind ": " #Class);
+
----------------
mizvekov wrote:
> davrec wrote:
> > mizvekov wrote:
> > > davrec wrote:
> > > > mizvekov wrote:
> > > > > davrec wrote:
> > > > > > Could we just return `X` here? Would that just default to the old
> > > > > > behavior instead of crashing whenever unforeseen cases arise?
> > > > > No, I think we should enforce the invariants and make sure we are
> > > > > handling everything that can be handled.
> > > > >
> > > > > Classing `TemplateTypeParm` as sugar-free was what was wrong and we
> > > > > missed this on the review.
> > > > There might always going to be a few rare corner cases vulnerable to
> > > > this though, particularly as more types are added and the people adding
> > > > them don't pay strict attention to how to incorporate them here, and
> > > > don't write the requisite tests (which seem very difficult to foresee
> > > > and produce). When those cases arise we will be crashing even though
> > > > we could produce a perfectly good program with the intended semantics;
> > > > the only thing that would suffer for most users is slightly less clear
> > > > diagnostic messages for those rare cases. I think it would be better
> > > > to let those cases gradually percolate to our attention via bug reports
> > > > concerning those diagnostics, rather than inconveniencing the user and
> > > > demanding immediate attention via crashes. Or am I missing something?
> > > I could on the same argument remove all asserts here and just let the
> > > program not crash on unforeseen circumstances.
> > >
> > > On the other hand, having these asserts here helps us catch bugs not only
> > > here, but in other parts of the code. For example uniquing /
> > > canonicalization bugs.
> > >
> > > If someone changes the properties of a type so that the assumptions here
> > > are not valid anymore, it's helpful to have that pointed out soon.
> > >
> > > Going for as an example this specific bug, if there werent those asserts
> > > / unreachables in place and we had weakened the validation here, it would
> > > take a very long time for us to figure out we were making the wrong
> > > assumption with regards to TemplateTypeParmType.
> > >
> > > I'd rather figure that out sooner on CI / internal testing rather than
> > > have a bug created by a user two years from now.
> > Yes its nicer to developers to get stack traces and reproductions whenever
> > something goes wrong, and crashing is a good way to get those, but users
> > probably won't be so thrilled about it. Especially given that the main
> > selling point of this patch is that it makes diagnostics nicer for users:
> > isn't it a bit absurd to crash whenever we can't guarantee our diagnostics
> > will be perfect?
> >
> > And again the real problem is future types not being properly incorporated
> > here and properly tested: i.e. the worry is that this will be a continuing
> > source of crashes, even if we handle all the present types properly.
> >
> > How about we leave it as an unreachable for now, to help ensure all the
> > present types are handled, but if months or years from now there continue
> > to be crashes due to this, then just return X (while maybe write something
> > to llvm::errs() to encourage users to report it), so we don't make the
> > perfect the enemy of the good.
> It's not about crashing when it won't be perfect. We already do these kinds
> of things, see the FIXMEs around the TemplateArgument and
> NestedNameSpecifier, where we just return something worse than we wish to,
> just because we don't have time to implement it now.
>
> These unreachables and asserts here are about testing / documenting our
> knowledge of the system and making it easier to find problems. These should
> be impossible to happen in correct code, and if they do happen because of
> mistakes, fixing them sooner is just going to save us resources.
>
> `llvm::errs` suggestion I perceive as out of line with current practice in
> LLVM, we don't and have a system for producing diagnostics for results
> possibly affected by FIXME and TODO situations and the like, as far as I
> know, and I am not aware of any discussions in that direction. I expect a lot
> of complexity and noise if we went this way.
> And I think this would have even less chance of working out if we started to
> incorporate the reporting of violation of invariants into that.
>
> I think even just using raw `llvm::errs` on clang would be incorrect per
> design, and could likely break users that parse our output.
>
> I think if months and years from now, if someone stumbled upon an assert
> firing here and, instead of understanding what is happening and then fixing
> the code, just removed / weakened the assert, that would simply not be good
> and I hope a reviewer would stop that from happening :)
I tend to agree that an assertion is appropriate for this. But you could turn
this into
```
assert(false && "...");
return X;
```
which would still assert, but fall back to something "reasonable" in builds
without assertions. The issue with `llvm_unreachable` is that it translates to
`__builtin_unreachable()` in builds without assertions, and the optimizer takes
advantage of that quite heavily. That's why `llvm_unreachable` is better left
to places where we're pretty sure they're unreachable unless something went
horribly wrong, such as after switches that handle all enumeration values.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111283/new/
https://reviews.llvm.org/D111283
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
