aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:12395-12398
+ // TODO: This should probably just be llvm_unreachable and we should
+ // reject triples with random ABIs and such when we build the target.
+ // For now, crash.
+ llvm::report_fatal_error("Unhandled environment in triple");
----------------
bogner wrote:
> aaron.ballman wrote:
> > Hmmm, is this going to be done as a follow-up relatively soon?
> > `report_fatal_error` isn't acceptable in lieu of actual diagnostic
> > reporting, so this should either be fixed in this patch or Really Soon
> > After™ it lands.
> Yeah, I'm happy to go and tackle that this week. Really I want an
> assert/unreachable here but since it is in fact reachable at the moment this
> seemed better as a temporary fix. Note that before this patch we just index
> out of bounds of an array and get all of the fun benefits of UB.
SGTM to do it as a follow-up -- this is better than the out of bounds indexing!
================
Comment at: clang/lib/Sema/SemaDecl.cpp:12423
+ << NT << HLSLShaderAttr::ConvertShaderTypeToStr(ST)
+ << "compute, amplification, or mesh";
+ FD->setInvalidDecl();
----------------
bogner wrote:
> aaron.ballman wrote:
> > This should be part of the diagnostic wording itself (using `%select`)
> > rather than passed in as a string argument. The same suggestion applies
> > elsewhere -- we want to keep English strings out of the streaming arguments
> > as much as possible to make localization efforts possible.
> That's a good point, though in this case the words we want are the value to
> the `compute(...)` attribute or part of the triple, so translating them
> wouldn't really make sense. Would it be better to do something using
> `ConvertShaderTypeToStr`? Also I'll probably update the error message text to
> quote the other use of ConvertShaderTypeToStr so it's clear that it isn't
> just a random word.
Doing something like `ConvertShaderTypeToStr()` would work as well -- it's
mostly an effort to avoid having random string literals with the calls to
`Diag()`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158803/new/
https://reviews.llvm.org/D158803
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
