aaron.ballman added inline comments.
================ Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80 + ParsedAttributes Attrs(AttrFactory); + MaybeParseCXX11Attributes(Attrs); + MaybeParseMicrosoftAttributes(Attrs); + ---------------- beanz wrote: > aaron.ballman wrote: > > 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) > @aaron.ballman I think those are all good questions. Generally HLSL has used > Microsoft attribute syntax, and I've started extending the Clang support to > be more robust, but still have more work to do. > > More on this patch, I want to take a step back. > > I think @python3kgae copied this code from DXC, but I don't think it is ever > used. I don't think we have any attributes in the language that are valid > with cbuffer or tbuffer subjects. We certainly don't have any attributes > implemented in clang that would be valid on these targets. > > That makes me think we should remove since it should be dead and unreachable > and untestable code. > > Since these HLSL buffer decls are an older (although frequently used) HLSL > feature, I think our general preference is to not extend new feature support > to them, and instead to encourage users to use the newer buffer types. > > Does that sound reasonable? > We certainly don't have any attributes implemented in clang that would be > valid on these targets. Despite knowing nothing about HLSL, I feel like pushing back a little bit here: deprecated, nodiscard, maybe_unused, and many others seem like they'd not only be valid on the target but perhaps useful to users. > Does that sound reasonable? I'm totally fine with that approach; we can debate attributes later. :-) 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
