[PATCH] D63954: Add lifetime categories attributes

Mon Jul 1 11:10:30 PDT 2019

xazax.hun added a comment.

In D63954#1565109 <https://reviews.llvm.org/D63954#1565109>, @gribozavr wrote:

> > I agree. In a follow-up patch we will add the attributes to STL types during parsing. Do you think it is OK to always add the attributes or should we only do it if a flag, e.g.  -wlifetime is added to the compiler invocation?
>
> Warning flags should not change the AST -- compiler engineers don't expect that. So if Clang is going to perform inference, it should either always do it, or it should be guarded by an `-f` flag, not a `-W` flag.

Thanks, this make sense. My only concern would be that a -W flag without the "inference" for STL types would be useless. Is it ok to make the driver add a -f flag automatically if a warning is turned on or would you find that confusing as well?

> 
> 
>> On the other hand I still think it is useful to give the users the option to maintain a header with forward declarations to add the annotations to other (non-standard) 3rd party types. These headers might be fragile to 3rd party changes but could still be a better option than maintaining patches on top of 3rd party libraries. Having API notes upstream would be a superior solution here and I might look into upstreaming it at some point, but I think this is something that could be addressed later on. What do you think?
> 
> I think it is acceptable for 3rd party libraries, but there's already a solution for 3rd party libraries -- API notes in Swift's fork of Clang. It has been successfully used by Apple for 5 years for this exact purpose (adding attributes to existing libraries without changing headers), and only needs to be upstreamed to Clang.

Supporting the forward declarations way is only a few lines of code now, supporting API Notes is a much larger effort. I would also prefer the latter but I think having this work not blocked on upstreaming API notes is essential to get this upstreamed. Or would you prefer not supporting the 3rd party library use-case until API Notes are upstreamed?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954