[PATCH] D158803: [Sema][HLSL] Consolidate handling of HLSL attributes

Mon Aug 28 09:48:12 PDT 2023

bogner 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");
----------------
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.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:12423
+          << NT << HLSLShaderAttr::ConvertShaderTypeToStr(ST)
+          << "compute, amplification, or mesh";
+      FD->setInvalidDecl();
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158803