[PATCH] D130033: [HLSL] Add resource binding attribute for HLSL.

[Xiang Li via Phabricator via cfe-commits]

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