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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6557-6558
+Here're resource binding examples with and without space:
+``RWBuffer<float> Uav : register(u3, space1)``
+``Buffer<float> Buf : register(t1)``
+The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3d12/resource-binding-in-hlsl
----------------
Two things:

* The example doesn't use valid RST, it should look more like what's done on AttrDocs:649 (using `.. code-block:: c++`)
* I think the example is missing semicolons at the end of the declarations?


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:112-113
+    }
+    if (!Tok.is(tok::identifier)) {
+      Diag(Tok.getLocation(), diag::err_expected) << tok::identifier;
+      SkipUntil(tok::r_paren, StopAtSemi); // skip through )
----------------
python3kgae wrote:
> aaron.ballman wrote:
> > Any reason not to use `ExpectAndConsume(tok::identifier, diag::err_expected)` here instead?
> Need to save the identifier string and loc for later use.
> And the consume is done in ParseIdentifierLoc.
Ah, that's a good reason, thanks!


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2893-2894
+  else if (const auto *RB = dyn_cast<HLSLResourceBindingAttr>(Attr))
+    NewAttr =
+        S.mergeHLSLResourceBindingAttr(D, *RB, RB->getSlot(), RB->getSpace());
   else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, Attr))
----------------
I don't see any tests covering this code path. How do you get such a redeclaration in HLSL?


================
Comment at: clang/test/SemaHLSL/resource_binding_attr_error.hlsl:15
+// expected-error at +1 {{invalid space specifier 's2' used; expected 'space' followed by an integer, like space1}}
+cbuffer a : register(b0, s2) {
+
----------------
Isn't this a re-definition of `a` which would cause an error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130033