aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
There's a whole ton of test coverage missing from this (no actual sema tests,
no AST dumping tests for text or JSON) not to mention a ton of situational
tests that are missing. For example, can you put one of these declarations
inside of a struct (so it's a member declaration)? How about declaring one as a
local variable? Tests that it properly diagnoses use of a template, as in
`template <typename Ty> cbuffer a { Ty f; };`? Are there restrictions on what
can be declared within one of these buffers (can I declare an inline function
in one?)? All those sorts of situations and more should be tested explicitly.
================
Comment at: clang/include/clang/AST/Decl.h:4675
+/// HLSLBufferDecl - Represent a cbuffer or tbuffer declaration.
+class HLSLBufferDecl : public NamedDecl, public DeclContext {
+ /// LBraceLoc - The ending location of the source range.
----------------
Are these redeclarable declarations? I assume not based on previous
discussions, so mostly just double checking.
================
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);
----------------
Speculative question: would it make more sense to add `CreateCBuffer` and
`CreateTBuffer` static methods rather than use a `bool` parameter?
================
Comment at: clang/include/clang/AST/Decl.h:4696
+
+ virtual SourceRange getSourceRange() const LLVM_READONLY {
+ return SourceRange(getLocStart(), RBraceLoc);
----------------
Nope. :-D
================
Comment at: clang/include/clang/AST/Decl.h:4699-4700
+ }
+ const char *getDeclKindName() const;
+ SourceLocation getLocStart() const LLVM_READONLY { return KwLoc; }
+ SourceLocation getRBraceLoc() const { return RBraceLoc; }
----------------
Unused and unimplemented.
================
Comment at: clang/lib/AST/Decl.cpp:5228-5230
+ if (DC != LexicalParent) {
+ Result->setLexicalDeclContext(LexicalParent);
+ }
----------------
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?
================
Comment at: clang/lib/AST/DeclBase.cpp:1197-1198
- return getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export;
+ return getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export ||
+ getDeclKind() == Decl::HLSLBuffer;
}
----------------
================
Comment at: clang/lib/AST/DeclPrinter.cpp:466-472
} else if (isa<NamespaceDecl>(*D) || isa<LinkageSpecDecl>(*D) ||
isa<ObjCImplementationDecl>(*D) ||
isa<ObjCInterfaceDecl>(*D) ||
isa<ObjCProtocolDecl>(*D) ||
isa<ObjCCategoryImplDecl>(*D) ||
- isa<ObjCCategoryDecl>(*D))
+ isa<ObjCCategoryDecl>(*D) ||
+ isa<HLSLBufferDecl>(*D))
----------------
Let's make this better while we're touching it.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:1664-1668
+ if (D->isCBuffer()) {
+ Out << "cbuffer ";
+ } else {
+ Out << "tbuffer ";
+ }
----------------
================
Comment at: clang/lib/AST/TextNodeDumper.cpp:2390
+
+void TextNodeDumper::VisitHLSLBufferDecl(const HLSLBufferDecl *D) {
+ if (D->isCBuffer())
----------------
You're missing similar changes for the `JSONNodeDumper`.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1793
+ case tok::kw_tbuffer:
+ SingleDecl = ParseHLSLBuffer(DeclEnd);
+ break;
----------------
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.
================
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);
+ }
----------------
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.
};
```
================
Comment at: clang/lib/Sema/SemaHLSL.cpp:27
+
+ ActOnDocumentableDecl(Result);
+
----------------
I don't see any tests for this.
================
Comment at: clang/lib/Sema/SemaHLSL.cpp:33
+void Sema::ActOnFinishHLSLBuffer(Decl *Dcl, SourceLocation RBrace) {
+ HLSLBufferDecl *BufDecl = dyn_cast_or_null<HLSLBufferDecl>(Dcl);
+ assert(BufDecl && "Invalid parameter, expected HLSLBufferDecl");
----------------
(The interface was deprecated recently and this is the replacement API.)
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:877
+Decl *TemplateDeclInstantiator::VisitHLSLBufferDecl(HLSLBufferDecl *Decl) {
+ llvm_unreachable("HLSL buffer declarations cannot be instantiated");
+}
----------------
Is this actually unreachable?
================
Comment at: clang/test/AST/HLSL/cbuffer_tbuffer.hlsl:7
+ float a;
+}
+
----------------
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.
================
Comment at: clang/test/SemaHLSL/cb_error.hlsl:3-21
+// expected-error@+2 {{expected identifier}}
+// expected-error@+1 {{expected unqualified-id}}
+cbuffer { ... };
+// expected-error@+1 {{expected '{'}}
+cbuffer missing_definition;
+// expected-error@+1 {{expected unqualified-id}}
+int cbuffer;
----------------
None of these are Sema tests, they're all parsing tests. I think the content
should be moved to the parsing test directory and sema tests should be added.
For example, we should clearly test using the same identifiers for these
buffers and comment about why that's valid and not an error. We should also
have tests that demonstrate name lookup behavior, e.g., what happens with this:
```
cbuffer a {
int x;
};
int main() {
return sizeof(a);
}
```
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
