[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I think that I have addressed all open comments.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:4164
+  let Content = [{
+When annotating a class ``O`` with ``[[gsl::Owner(T)]]``, then each function
+that returns cv-qualified ``T&`` or ``T*`` is assumed to return a
----------------
gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > Slightly better: "In a class annotated with ..., each function that returns ... is assumed to ..."
> > > 
> > > However, the "is assumed" part throws me off. There's no assumption here -- this is what this attribute means, intentionally, by design, no guessing involved.
> > > 
> > > So how about this?
> > > 
> > > The attribute `[[gsl::Owner(T)]]` applies to structs and classes that own an object of type `T`:
> > > 
> > > ```
> > > [[gsl::Owner(int)]] class IntOwner {
> > > private:
> > >   int value;
> > > public:
> > >   int *getInt() { return &value; }
> > >   long *getLong();
> > > };
> > > ```
> > > 
> > > In a class annotated like this each member function that returns `T&` or `T*` returns a pointer/reference to the data owned by the class instance:
> > > 
> > > ```
> > > IntOwner int_owner;
> > > int *x = int_owner.getInt();
> > > // `x` points to memory owned by `int_owner`.
> > > ```
> > > 
> > > Therefore, that the lifetime of that data ends when ... For example:
> > > 
> > > ```
> > > int *x = IntOwner().getInt();
> > > // `x` points to memory owned by the temporary `IntOwner()`, which has been destroyed now.
> > > 
> > > long *y = IntOwner().getLong();
> > > // Can't make any conclusion about validity of `y`.
> > > ```
> > > 
> > What I meant to express by "is assumed to" is that this attribute does not change what member functions do. E.g. if I had `int* f() { static int i; return &i; }`, then `f` would not return data owned by the class instance, even when annotating the class with `[[gsl::Owner(T)]]`. The point is that when member functions with return type `T*`/`T&` don't return data owned by the class instance, one should not add the `[[gsl::Owner(T)]]` attribute - or the analysis will produce wrong results. Here, we would warn if code uses the returned value of `f` after the class instances has been destroyed even though it would be safe.
> > 
> > There is a chicken and egg problem here. With this PR, we don't get any analysis yet, so providing examples of analysis results like "`x` points to memory owned by the temporary `IntOwner()`, which has been destroyed now." can be misleading. On the other hand, if we would just document the current meaning of the attribute, it would be nothing.
> > The implication "member function that returns `T&` or ` T*` returns a pointer/reference to the data owned by the class instance" is just one part of being an Owner. Herb's paper
> > uses this in more ways (e.g. output arguments, handling of move semantics, etc.), which we would implement incrementally together with the respective documentation update.
> > 
> > So I propose to keep the documentation basic for this PR and update the documentation with the upcoming PRs that add to the analysis. Would that be acceptable?
> > I added a note that the attribute is currently experimental and subject to change.
> > What I meant to express by "is assumed to" is that this attribute does not change what member functions do.
> 
> I don't think we should be making allowances for code that uses an attribute to promise something and then does some other thing. In that model, there's alignment between what code does and what attribute says.
> 
> And I mean of course the attribute does not change what the code does... that must be obvious but you can mention it explicitly if you want.
> 
> > With this PR, we don't get any analysis yet
> 
> I don't think the analysis must be automatic in order to show what analysis can be done. Attribute documentation needs enough examples to give people a flavor of what the attribute does, and what possible benefits it can bring.
> 
> Documentation for the specific analysis that is actually done should go elsewhere into Clang's documentation (if necessary).
I added an example to the documentation to show how the initial analysis will look like to give a feeling for what the attribute does.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+    S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << AL;
+    return;
----------------
gribozavr wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > Is `void` really the only problematic type? What about incomplete types, for instance?
> > I think incomplete types are fine. The only information we actually need here is the name the pointee types. E.g. for a `SomeOwner<Incomplete>` which is annotated `Owner(Incomplete)`, we will assume that a method returning `Incomplete*`, `std::reference_wrapper<Incomplete>` etc will return an object pointing to the memory owned by `SomeOwner`.
> What about array types? References?
> 
> gsl::Owner(int[])
> gsl::Owner(int&)
Good catch! I disallow them now and added appropriate tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954