aaron.ballman added a comment. In D121283#3375806 <https://reviews.llvm.org/D121283#3375806>, @egorzhdan wrote:
> In D121283#3373560 <https://reviews.llvm.org/D121283#3373560>, @aaron.ballman > wrote: > >> why do we support multiple attribute *specifiers* in the same pragma? I >> would not expect to be able to mix attribute styles in the same pragma given >> that all of the individual styles allow you to specify multiple attributes >> within a single specifier > > I don't think I have a strong use case for this. It seemed consistent with > the way multiple attributes can be added for individual declarations, e.g. > `__attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f()`. But we > can prohibit multiple specifiers within a single pragma if you think that > this is not a good construct to support. I don't yet think it's a *bad* construct to support... > In D121283#3373560 <https://reviews.llvm.org/D121283#3373560>, @aaron.ballman > wrote: > >> why is whitespace the correct separator as opposed to a comma-delimited list? > > My motivation for this was also consistency with the syntax for attributes in > individual declarations. Given that attribute specifiers are delimited by > space for individual declarations (`__attribute__((cdecl)) > __attribute__((noreturn)) void f()`), I think it would be unintuitive to > require commas for attribute specifiers in pragmas when we could instead > reuse the existing syntax of space-delimited attribute specifiers. Thanks, that makes some sense to me, but at the same time, I can't think of another pragma that behaves similarly off the top of my head (usually, lists of things have a delimiter other than whitespace), so it's kind of unintuitive either way. As a thought experiment, would it make sense to lift the restriction on the number of attributes allowed in a pragma, but not allow multiple attribute specifiers? e.g., // These are fine #pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) #pragma clang attribute pop #pragma clang attribute push (__attribute__((noreturn, noinline)), apply_to = function) #pragma clang attribute pop // These are not allowed #pragma clang attribute push ([[clang::disable_tail_calls]] [[noreturn]], apply_to = function) #pragma clang attribute pop #pragma clang attribute push (__attribute__((noreturn)) __attribute__((noinline)), apply_to = function) #pragma clang attribute pop #pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((noreturn)), apply_to = function) #pragma clang attribute pop This still allows you to specify more than one attribute in the pragma, but removes the concerns about how to separate the syntaxes (whitespace or another delimiter) while still leaving that design open in the future if there's a strong need to mix syntaxes. The pragma attribute feature has a lot of power to it, but it also comes with a lot of risk to users, so it's a feature that I think we should be cautious about extending (in general). I think what you propose here could very well be reasonable, but I'm a bit worried that there's not a clear need for that amount of flexibility, which is why I'm sort of leaning towards being more restrictive here. However, this isn't exposing any functionality that users can't already do the long way with multiple pragmas, so I don't see a blocking concern with the current patch either. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121283/new/ https://reviews.llvm.org/D121283 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
