xazax.hun marked an inline comment as done. xazax.hun added inline comments.
================ Comment at: clang/lib/Sema/SemaType.cpp:7727 + // Move function type attribute to the declarator. + case ParsedAttr::AT_LifetimeContract: ---------------- aaron.ballman wrote: > xazax.hun wrote: > > aaron.ballman wrote: > > > This is not an okay thing to do for C++ attributes. They have a specific > > > meaning as to what they appertain to and do not move around to better > > > subjects like GNU-style attributes. > > Yeah, I know that. But this is problematic in the standard. See the > > contracts proposal which also kind of violates this rule by adding an > > exception. Basically, this is the poor man's version of emulating the > > contracts proposal. In the long term we plan to piggyback on contracts > > rather than hacking the C++ attributes. > The contracts proposal was not adopted and I am opposed to adding this change > for any standard attribute. We've done a lot of work to ensure that those > attributes attach to the correct thing based on lexical placement and I don't > want to start to undo that effort. I am not sure how to proceed in this case. As far as I understand this wasn't the main reason why didn't we adopt the contracts proposal. While I do understand why is it good to make the lexical placement matter, but the current placement hinders non-trivial use-cases. Currently, we either have a type attribute or we cannot mention any of the formal parameters. This is why the contracts cannot work without changing attributes in the standard, and the same reason why this patch will never work without doing this. Or do you have an alternative in mind? Do you think introducing a frontend flag for this attribute would help? This would make it clear that we are currently using a non-standard feature that needs to be enabled explicitly. Again, this is a temporary solution, that will go away as soon as we have contracts. Since contracts need to solve the same problem, whatever the solution will be, this patch can piggyback on that. What do you think? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72810/new/ https://reviews.llvm.org/D72810 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
