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

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked an inline comment as done.
python3kgae added inline comments.


================
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 )
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6992
+
+  if (!Space.startswith_insensitive("space")) {
+    S.Diag(SpaceArgLoc, diag::err_hlsl_expected_space) << Space;
----------------
aaron.ballman wrote:
> I'm surprised we want to be case insensitive here; we don't document that we are, but also, we're not case insensitive with the slot letters so it seems inconsistent. Is this part of the HLSL requirements?
> 
> (Note, I have no problems with doing a case insensitive check if the case sensitive check fails so we can give a better warning + fixit, if you thought that was useful.)
Discussed with the team, we'll limit to lower case now.
If need for case insensitive for back-compat in the future, it is easy to enable it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130033