aaron.ballman added a comment.
Thanks for the update, this is heading in a good direction! I think there's
still a lot of test coverage missing. Consider:
namespace Name {
cbuffer a {
int x;
}
}
struct S {
cbuffer what {
int y;
}
};
void func() {
tbuffer derp {
int z;
}
decltype(derp) another {
int a;
}
}
================
Comment at: clang/include/clang/AST/Decl.h:4690-4693
+ static HLSLBufferDecl *Create(ASTContext &C, DeclContext *LexicalParent,
+ bool CBuffer, SourceLocation KwLoc,
+ IdentifierInfo *ID, SourceLocation IDLoc,
+ SourceLocation LBrace);
----------------
aaron.ballman wrote:
> Speculative question: would it make more sense to add `CreateCBuffer` and
> `CreateTBuffer` static methods rather than use a `bool` parameter?
I'm still on the fence about this. On the one hand, being explicit is far
better than hoping the reader understand the bool argument at the call site. On
the other hand, using the bool parameter means less branching at the call
sites. So I'm leaning towards leaving this as-is, but if @beanz has other
opinions, I don't yet have a strong feeling.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1608
"expected HLSL Semantic identifier">;
+def err_invalid_declaration_in_hlsl_buffer : Error<"invalid declaration inside
cbuffer/tbuffer">;
def err_unknown_hlsl_semantic : Error<"unknown HLSL semantic %0">;
----------------
You know what kind of buffer it's within so we can be more precise.
================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:904
+ VisitNamedDecl(D);
+ JOS.attribute("buffer_kind", D->isCBuffer() ? "cbuffer" : "tbuffer");
+}
----------------
Matches the naming style for most everything else in the file.
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:20-21
+Decl *Parser::ParseHLSLBuffer(SourceLocation &DeclEnd,
+ SourceLocation InlineLoc) {
+ assert((Tok.is(tok::kw_cbuffer) || Tok.is(tok::kw_tbuffer)) &&
----------------
Parameter is unused, I presume that's intentional rather than accidental?
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:25
+ bool IsCBuffer = Tok.is(tok::kw_cbuffer);
+ SourceLocation BufferLoc = ConsumeToken(); // eat the 'cbuffer or tbuffer'.
+
----------------
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:33
+ IdentifierInfo *Identifier = Tok.getIdentifierInfo();
+ SourceLocation IdentifierLoc = ConsumeToken(); // consume identifier
+
----------------
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:50-54
+ // FIXME: support attribute on constants inside cbuffer/tbuffer.
+ ParsedAttributes Attrs(AttrFactory);
+ ParsedAttributes DeclSpecAttrs(AttrFactory);
+
+ if (PP.isCodeCompletionReached()) {
----------------
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:59
+
+ switch (Tok.getKind()) {
+ case tok::kw_namespace:
----------------
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.
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:64-65
+ T.skipToEnd();
+ return nullptr;
+ break;
+ case tok::kw_cbuffer:
----------------
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:71-74
+ return nullptr;
+
+ [[fallthrough]];
+ case tok::annot_pragma_vis:
----------------
There's no way to reach this, so we shouldn't need the `[[fallthrough]]` here,
right?
================
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:
----------------
Why are type aliases prohibited?
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:106-107
+ case tok::kw_template:
+ case tok::kw_static_assert:
+ case tok::kw__Static_assert:
+ case tok::kw_inline:
----------------
Why are static assertions prohibited?
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:122-123
+ case tok::kw_static:
+ // Parse (then ignore) 'static' prior to a template instantiation. This
+ // is a GCC extension that we intentionally do not support.
+ if (getLangOpts().CPlusPlus && NextToken().is(tok::kw_template)) {
----------------
1) GCC supports HLSL?
2) This seems wrong because it will result in:
```
static template <typename Ty> Ty Val{}; // Prohibited
static void func(); // Allowed
inline void other(); // Prohibited
static inline void haha(); // Allowed
```
As mentioned above, I think we should use general parsing mechanisms here and
then look at the AST node generated to see whether it's an allowed one or not.
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:134
+ }
+ ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+ DeclSpecAttrs, true, nullptr, &Loc);
----------------
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 {
...
}
}
```
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:134-135
+ }
+ ParseSimpleDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+ DeclSpecAttrs, true, nullptr, &Loc);
+ break;
----------------
You're parsing things and then dropping them on the floor?
================
Comment at: clang/test/ParserHLSL/invalid_inside_cb.hlsl:20-24
+ -
+}
+cbuffer A {
+// expected-error@+1 {{invalid declaration inside cbuffer/tbuffer}}
+ +
----------------
These aren't declarations or even really close (maybe if you squint and think
about Objective-C?) so the diagnostic here is really weird. I'd expect to be
some sort of syntax error because we have no idea what to parse.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129883/new/
https://reviews.llvm.org/D129883
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
