[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:7727
 
+    // Move function type attribute to the declarator.
+    case ParsedAttr::AT_LifetimeContract:
----------------
xazax.hun wrote:
> 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?
> 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.

There were a lot of flaws, and this was one of them. I don't think you could point to any one flaw and say "this is why we didn't get contracts" though.

> Or do you have an alternative in mind?

I agree that this is an existing deficiency in both the C and C++ standards with the attribute syntax for functions. There simply is no way to make a function declaration attribute that can refer to the parameters currently. The only alternative that comes to mind is to use only a GNU-style spelling rather than trying to use a [[]] spelling. 

> 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.

There's no guarantee that contracts will come back with the same syntax used previously, so I wouldn't want to rely on that to solve the issue until we have a better idea of what contracts will look like coming out of the new study group. My preference is to use the GNU-style spelling and not provide a [[]] spelling. We've done that for other attributes (like `enable_if` and `diagnose_if`.


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

https://reviews.llvm.org/D72810