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

[Xiang Li via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:59
+
+    switch (Tok.getKind()) {
+    case tok::kw_namespace:
----------------
aaron.ballman wrote:
> The approach of using a switch and handling individual keywords specially strikes me as being quite fragile. I think a better approach is to split this loop out into its own function and model it (at least somewhat) after `ParseStructUnionBody()`.
> 
> The current approach worries me because I have no idea why there's a giant block of invalid things or what should be added to that as we add new declarations to the compiler (and certainly nobody is going to think to come update this function when adding support for new kinds of declarations.
Changed to ParseExternalDeclaration then validate that only Function/Record/Var Declarations are in the result.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:103-104
+    case tok::kw_export:
+    case tok::kw_using:
+    case tok::kw_typedef:
+    case tok::kw_template:
----------------
aaron.ballman wrote:
> Why are type aliases prohibited?
cbuffer is a legacy feature for HLSL while type alias is a new feature for HLSL2021.
The plan is to keep the legacy features as is.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:134
+      }
+      ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+                             DeclSpecAttrs, true, nullptr, &Loc);
----------------
python3kgae wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > You're parsing things and then dropping them on the floor?
> > The declarator context looks wrong to me -- I don't see anything prohibiting the user from doing:
> > ```
> > namespace N {
> > cbuffer {
> >   ...
> > }
> > }
> > ```
> > 
> The goal is to add these things to HLSLBuffer DeclarationContext.
> ActOnStartHLSLBuffer should already make sure that.
namespace N {
  cbuffer A {
  }
}
is supported in HLSL.

cbuffer A {
  namespace N {
  }
} is tricky to support because the namespace N decl inside cbuffer needs to be accessed by things outside the cbuffer. This is not supported now and I hope we don't need to support it in the future.

I'll add test for the supported case.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:134-135
+      }
+      ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+                             DeclSpecAttrs, true, nullptr, &Loc);
+      break;
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > You're parsing things and then dropping them on the floor?
> The declarator context looks wrong to me -- I don't see anything prohibiting the user from doing:
> ```
> namespace N {
> cbuffer {
>   ...
> }
> }
> ```
> 
The goal is to add these things to HLSLBuffer DeclarationContext.
ActOnStartHLSLBuffer should already make sure that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129883