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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
