[PATCH] D121283: [Clang] Support multiple attributes in a single pragma

[Aaron Ballman via Phabricator via cfe-commits]

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