aaron.ballman added a comment. In D112773#3097730 <https://reviews.llvm.org/D112773#3097730>, @beccadax wrote:
> 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. Oohhhh, that is not what my expectations were. I thought we would be applying it to anything in this situation. That we're not is possibly a bug, but I happen to like the behavior in this case. >> 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. That sounds like a good reason to not give it a subject list. > 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. I'd like some confirmation of that. My reading of https://clang.llvm.org/docs/LanguageExtensions.html#specifying-an-attribute-for-multiple-declarations-pragma-clang-attribute suggests that `any()` /could/ apply to everything. (`The any rule applies attributes to all declarations that are matched by at least one of the rules in the any.`) My recollection of the discussions when we adopted #pragma clang attribute is that we tried our best to limit the damage from the pragma over-applying attributes, but I don't recall if we explicitly talked about any() with no subjects. What would make me comfortable with this patch is 1) doing some archeology to find the original reviews for pragma clang attribute to see if we made any sort of explicit decision here but forgot to document it. If we didn't make an explicit decision in this area, we should make one now. We can either decide "any() with no subjects applies to everything", "any() with no subjects is dangerous and will apply to nothing", or "any() with no subjects is dangerous and we will diagnose if the user tries this" (probably other ideas we could consider). Ultimately, I would like to see the pragma clang attribute documentation updated before/when we land this bit so that our documentation clearly sets user expectations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112773/new/ https://reviews.llvm.org/D112773 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
