mizvekov added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:12367
+ case Type::Class:
\
+ llvm_unreachable("Unexpected " Kind ": " #Class);
+
----------------
aaronpuchert wrote:
> 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.
I see, that is reasonable.
But grepping for `assert(false` in the code base, I see that they are pretty
rare, accounting for a bit less than 0.5% in `clang/` and 0.3% in `llvm/`,
compared to the total number of asserts. They occur fifty times less than plain
llvm_unreachables. I don't remember ever seeing them in practice, so I was not
aware this was something we would do.
I feel like in these cases where we are using llvm_unreachable in this patch, I
am 100% sure they are really unreachable, unless something went horribly wrong.
But I mean I could be completely wrong as well, I must admit, so maybe in those
cases that is what went horribly wrong? (Me being wrong I mean)
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
