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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+    ParsedAttributes Attrs(AttrFactory);
+    MaybeParseCXX11Attributes(Attrs);
+    MaybeParseMicrosoftAttributes(Attrs);
+
----------------
beanz wrote:
> python3kgae wrote:
> > aaron.ballman wrote:
> > > Just double-checking, but this allows `[[]]` style attributes as well as `[]` style attributes, but not `__attribute__` or `__declspec` style attributes, is that intended?
> > That is what dxc currently support.
> > It may change in the future. But for now, only [[]] and [] are supported.
> Well... worth noting, HLSL doesn't actually support C++11 attributes, but that is almost certainly going to change in the near future, so we might as well support them from the start in Clang.
Ah, that is good to know @beanz -- we should think carefully about this situation because there are some tradeoffs to consider.

1) It's pretty weird to support half of the Microsoft attribute syntax (and the half we barely have any attribute support for, at that). Is there a reason to not support `__declspec` as well? (For example, are there concerns about things like using those attributes to do dllexport or specify a COMDAT section, etc?) In fact, if we're going to support vendor attributes like `[[clang::overloadable]]`, it's a bit weird that we then prohibit the same attribute from being spelled `__attribute__((overloadable))`, is there a particular reason to not extend to all attributes?
2) Supporting `[]` style attributes means that attribute order is important. We cannot use `MaybeParseAttributes()` to parse attribute specifiers in any order because `[]` causes ambiguities under some circumstances. So you're stuck with the order you have -- `[[]]` attributes first, `[]` attributes second. Is that ordering reasonable?

And for the patch itself -- there are no test cases demonstrating what happens when using attributes on things within one of these buffers. I expect many things to be quite reasonable, like using `[[deprecated]]`, but are the attributes which impact codegen reasonable as well? (Like naked functions, returns twice, disable tail calls, etc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129883