aaron.ballman added a reviewer: beanz.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:3978
+ "compute", "raygeneration", "intersection",
+ "anyhit", "closestHit", "miss", "callable", "mesh",
+ "amplification"],
----------------
`anyhit` vs `closestHit` in terms of capitalization seems a bit surprising to
me -- is that 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.
+ }];
----------------
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).
================
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">;
----------------
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.)
================
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;
+ }
----------------
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.)
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6961
+
+ // TODO: check function match the shader stage.
+
----------------
Are you planning to do this TODO as part of this patch, or is this a FIXME
someone needs to handle later?
================
Comment at: clang/test/SemaHLSL/shader_attr.hlsl:20
+// expected-warning@+1 {{'shader' attribute only applies to global functions}}
+ [shader("vertex")]
+static void oops() {}
----------------
Formatting looks off here.
Also, the diagnostic text leaves a bit to be desired (this is more of a general
problem with the `HLSLEntry` subject, so don't worry about it for your patch)
-- this is a global function.
================
Comment at: clang/test/SemaHLSL/shader_attr.hlsl:25
+// expected-warning@+1 {{'shader' attribute only applies to global functions}}
+ [shader("vertex")]
+static void oops() {}
----------------
Formatting looks off here as well. I'm guessing clang-format doesn't do
anything good with Microsoft-style attributes.
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
