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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
