[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked 5 inline comments as done.
python3kgae added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:5228-5230
+  if (DC != LexicalParent) {
+    Result->setLexicalDeclContext(LexicalParent);
+  }
----------------
aaron.ballman wrote:
> So semantically these are all in the global namespace no matter where they're spelled? e.g., if you put one inside of a namespace... it's still globally accessible?
It should not.
I was doing this to hoist nested buffer.
I'll drop the nested buffer support and make namespace work first.
Nested buffer support will be done in another review.



================
Comment at: clang/lib/Parse/ParseDecl.cpp:1793
+  case tok::kw_tbuffer:
+    SingleDecl = ParseHLSLBuffer(DeclEnd);
+    break;
----------------
aaron.ballman wrote:
> So these can form a declaration group? e.g.,
> ```
> cbuffer a { int b; }, b { float f; };
> ```
> I don't see any test coverage for this situation if that is supported. If it's not supported, I think this should be a `return` statement instead.
No, it cannot form a declaration group.
If early return, still need to convert to Decl group to match the return type of ParseDeclaration.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:48-52
+  while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
+    ParsedAttributes Attrs(AttrFactory);
+    // FIXME: support attribute on constants inside cbuffer/tbuffer.
+    ParseExternalDeclaration(Attrs);
+  }
----------------
aaron.ballman wrote:
> This looks like it's going to have some pretty bad error recovery behavior if something is amiss:
> ```
> cbuffer a {
>   oh no!
>   this isn't even valid HLSL code
>   despite seeming totally reasonable
>   once you understand that HLSL
>   is so flaming weird.
> };
> ```
Add more checks for the Tok here.


================
Comment at: clang/lib/Sema/SemaHLSL.cpp:27
+
+  ActOnDocumentableDecl(Result);
+
----------------
aaron.ballman wrote:
> I don't see any tests for this.
Removed. Not see any difference without it when -Wdocumentation is on.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:877
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+  llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}
----------------
aaron.ballman wrote:
> Is this actually unreachable?
Yeah. template on cbuffer/tbuffer is not allowed.


================
Comment at: clang/test/AST/HLSL/cbuffer_tbuffer.hlsl:7
+  float a;
+}
+
----------------
aaron.ballman wrote:
> Something's amiss -- this declaration doesn't have a semicolon, is that expected? Because the SemaHLSL tests do have terminating semicolons and no diagnostics about it being superfluous.
It is expected that cbuffer doesn't need a semicolon.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129883