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