[PATCH] D63954: Add lifetime categories attributes

Mon Jul 1 12:49:04 PDT 2019

gribozavr added a comment.

In D63954#1565128 <https://reviews.llvm.org/D63954#1565128>, @xazax.hun wrote:

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

Why not always attach attributes to standard library types? I only suggested the `-f` flag in case it is necessary to gate this functionality for some reason (e.g., performance).

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

I would prefer an API Notes-based approach. And I would suggest that you, or someone else working on these attributes, help with upstreaming.

The reason why I prefer API notes is because users forward declaring third party APIs will put those libraries in a difficult situation, where those libraries can't use normal evolution processes to change their APIs -- the forward declarations that users have will stop compiling, or will start introducing additional overloads which don't have definitions. Of course severity of the problem depends on the nature of such forward declarations -- would it only be for owner and pointer types, or for some other APIs? Just the types -- and rare types like pointers -- is not that bad, however forward declaring functions is bad.

With API notes users have all the extra stuff out of line, and it is easy to disable when things go sideways.

When forward declarations are sprinkled in users' code, they are impossible to change.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954