aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D112773#3135975 <https://reviews.llvm.org/D112773#3135975>, @beccadax wrote: >> 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 reading of that rule suggest that an `any()` with no subrules would not > match anything because, if you fed in a declaration, none of the > (nonexistent) subrules of the `any()` would match it. > >> 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. > > Looking at https://reviews.llvm.org/D30009, I don't see any relevant > discussion of how `any()` (without subrules) should behave. What I do see is > that they started with "match everything always" behavior, but concerns about > users not knowing which declarations it would be applied to (and how that > behavior might change as the compiler evolved) led them to the idea of > requiring users to list the declarations they expected the compiler to match > and having a way to allow a subset <https://reviews.llvm.org/D30009#679481>; > then they realized that only the subset version was actually useful > <https://reviews.llvm.org/D30009#706171>. What I take from this is that there > was clearly concern about users not being able to tell what the pragma would > affect and they changed the design so that users would need to specify it > clearly, which suggests that a way to leave it unspecified was seen as an > anti-goal. > > Looking at the implementation of > `Parser::ParsePragmaAttributeSubjectMatchRuleSet()`, it looks like `any` or > `any()` would cause a parse error—we return `true` (as in the error branches) > if we do not parse an open parenthesis or do not parse an identifier after > the open parenthesis—which again makes me think that the design intentionally > left out any mechanism to match all subjects. > > We don't have a statement that there is and should be no way to apply > `attribute push` to everything, but I think we can conclude that having such > a capability wasn't a goal of the final design. > > What would you like me to do from here? Thank you for diving into those details! I'm now sold on the idea that `any` with no subjects is dangerous and we will diagnose if the user tries this. So I think the only thing that needs to happen is a documentation update and some additional test coverage for this scenario, which I'm happy to take care of myself since it's orthogonal to these changes. Based on that, this LGTM as-is. 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
