[PATCH] D112773: Allow __attribute__((swift_attr)) in attribute push pragmas

[Becca Royal-Gordon via Phabricator via cfe-commits]

beccadax added a comment.

In D112773#3096185 <https://reviews.llvm.org/D112773#3096185>, @aaron.ballman wrote:

> `swift_attr` has no subjects, so this will attempt to spray the attribute onto literally *everything*. That makes this incredibly risky to use with the pragma approach (not to mention what it could do to memory consumption of large ASTs). I'm not certain I'm comfortable allowing this without an explicit set of subjects for the attribute.

What exactly is the risk here? In my testing, omitting `apply_to` or setting it to `any` or `any()` caused clang to apply the attribute to nothing, not everything. If there is a way I didn't find, perhaps we could address that by emitting a warning if you use the "match anything" subject with an attribute that has an empty subject list.

> Can we add subjects to this attribute to try to limit the damage somewhat?

I don't think we can, because `swift_attr` is by design very widely applicable—it allows you to apply arbitrary Swift language features to the imported representation of any Clang declaration. Any declaration Swift imports, including types and typedefs, functions, non-local variables, and parameters, ought to accept it.

Looking through the subject matchers available, almost everything would at least //theoretically// be a reasonable target for `swift_attr` (`variable(is_local)` is the only exception I noticed). We could restrict it to only declarations we're currently using it for, but many of these (e.g. C++ declarations) are likely to be supported in the future, so this would create clang churn and integration issues as Swift's capabilities expand.

So I don't think the risk you're worried about is really there, and I don't think adding a subject list would mitigate it very much. But if I've missed something, please point it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112773