python3kgae added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:4022
+ let LangOpts = [HLSL];
+ let Args = [StringArgument<"Slot", 1>, StringArgument<"Space", 1>];
+ let Documentation = [HLSLResourceBindingDocs];
----------------
aaron.ballman wrote:
> Do you really intend `Slot` to be optional?
Good catch.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11653-11654
+def err_hlsl_unsupported_register_number : Error<"register number should be an
integer">;
+def err_hlsl_expected_space : Error<"expected space argument specified as
'spaceX', where X is an integer">;
+def err_hlsl_unsupported_space_number : Error<"space number should be an
integer">;
----------------
aaron.ballman wrote:
> I don't know what the difference is between these two diagnostics; they seem
> to be the same situation. Also 'spaceX' looks like syntax the user wrote, but
> it's hardcoded in the diagnostic. Can these be replaced with `invalid space
> specifier '%0' used; expected 'space' followed by an integer` or something
> along those lines?
>
> (Do we want to be kind and give users a fix-it for something like `space 10`
> that removes the whitespace and recovers by pretending the user wrote
> `space10` instead? Similar question about whitespace for register type.)
Removed err_hlsl_unsupported_space_number.
The priority to add fix-it is very low compared to other things we need to add.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6969
+ } else {
+ if (Str.startswith_insensitive("space")) {
+ Space = Str;
----------------
aaron.ballman wrote:
> How do we get into this situation if the slot is mandatory?
It should not.
Fixed.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7009
+
+ // FIXME: check reg type match decl.
+ HLSLResourceBindingAttr *NewAttr =
----------------
aaron.ballman wrote:
> Any reason not to do that now?
Resource types like sampler/texture are ready yet.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130033/new/
https://reviews.llvm.org/D130033
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
