aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/AttrDocs.td:6557-6558 +Here're resource binding examples with and without space: +``register(t3, space1)`` +``register(t1)`` +The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3d12/resource-binding-in-hlsl ---------------- These aren't really fully examples of how to use this properly. Can you add an actual example (it doesn't have to do anything particularly useful, the goal is largely to show off how a user would use this attribute)? ================ 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 ) ---------------- Any reason not to use `ExpectAndConsume(tok::identifier, diag::err_expected)` here instead? ================ Comment at: clang/lib/Parse/ParseHLSL.cpp:138 + // Add numeric_constant for fix-it. + if (SpaceStr.equals_insensitive("space") && Tok.is(tok::numeric_constant)) + fixSeparateAttrArgAndNumber(SpaceStr, SpaceLoc, Tok, ArgExprs, *this, ---------------- Do we really mean insensitive here? (See comment below.) ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6942-6952 + StringRef Str; + SourceLocation ArgLoc; + if (!AL.isArgIdent(0)) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_type) + << AL << AANT_ArgumentIdentifier; + return; + } ---------------- ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6992 + + if (!Space.startswith_insensitive("space")) { + S.Diag(SpaceArgLoc, diag::err_hlsl_expected_space) << Space; ---------------- 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.) 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
