[PATCH] D123907: [HLSL] Add shader attribute

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3978
+                            "compute", "raygeneration", "intersection",
+                            "anyhit", "closestHit", "miss", "callable", "mesh",
+                            "amplification"],
----------------
aaron.ballman wrote:
> `anyhit` vs `closestHit` in terms of capitalization seems a bit surprising to me -- is that intentional?
Good catch. It is not intentional.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6386-6388
+The ``shader`` attribute applies to HLSL shader entry functions to identify the
+ shader stage for the entry function.
+  }];
----------------
aaron.ballman wrote:
> Please add enough documentation that a user has a chance to use this feature properly (and it's fine to link to other documentation for more details if there's canonical documentation elsewhere that we don't want to duplicate).
I'll fix this.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11598-11599
 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
+def err_hlsl_invalid_attribute_argument : Error<
+  "%0 attribute argument not supported: %1">;
 
----------------
aaron.ballman wrote:
> Almost every other attribute warns on this situation and then ignores the attribute; why should this be a hard error? (A warning allows other implementations to support more enumeration values without breaking the user's code in Clang.)
I'll change it to warning.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6944-6947
+  if (AL.getNumArgs() != 1) {
+    S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;
+    return;
+  }
----------------
aaron.ballman wrote:
> Hmm, doesn't the common attribute handling logic already deal with this case? (It does for other attribute syntaxes, but we have almost no attributes using the Microsoft `[]` syntax, so I wouldn't be surprised if we need to add that logic.)
I'll double check it I can remove this.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6961
+
+  // TODO: check function match the shader stage.
+
----------------
aaron.ballman wrote:
> Are you planning to do this TODO as part of this patch, or is this a FIXME someone needs to handle later?
It is a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123907