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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:3008-3011
+  void ActOnHLSLTopLevelFunction(FunctionDecl *FD);
   void CheckHLSLEntryPoint(FunctionDecl *FD);
+  void CheckHLSLSemanticAnnotation(FunctionDecl *EntryPoint, Decl *Param,
+                                   HLSLAnnotationAttr *AnnotationAttr);
----------------
Any const correctness you can add here to the pointees would be appreciated.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12375
+  if (HLSLShaderAttr::ConvertStrToShaderType(Env, ShaderType)) {
+    if (HLSLShaderAttr *Shader = FD->getAttr<HLSLShaderAttr>()) {
+      // The entry point is already annotated - check that it matches the
----------------
`const auto *`


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


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12420
+  case HLSLShaderAttr::Callable:
+    if (auto *NT = FD->getAttr<HLSLNumThreadsAttr>()) {
+      Diag(NT->getLoc(), diag::err_hlsl_attr_unsupported_in_stage)
----------------
`const auto *`


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


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12439-12440
 
-  for (const auto *Param : FD->parameters()) {
-    if (!Param->hasAttr<HLSLAnnotationAttr>()) {
+  for (ParmVarDecl *Param : FD->parameters()) {
+    if (auto *AnnotationAttr = Param->getAttr<HLSLAnnotationAttr>()) {
+      CheckHLSLSemanticAnnotation(FD, Param, AnnotationAttr);
----------------
const the pointees?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158803