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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
