[PATCH] D111283: [clang] template / auto deduction deduces common sugar

Tue Sep 13 06:14:00 PDT 2022

mizvekov added inline comments.

================
Comment at: clang/lib/AST/ASTContext.cpp:12367
+  case Type::Class:                                                            \
+    llvm_unreachable("Unexpected " Kind ": " #Class);
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111283/new/

https://reviews.llvm.org/D111283