mgehre added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+ let Spellings = [CXX11<"gsl", "Owner">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
aaron.ballman wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > Has Microsoft already added support for this? We had an unfortunate
> > > > > problem with `gsl::suppress` where we introduced the attribute first
> > > > > and MSVC introduced it under a different, incompatible syntax. I
> > > > > would strongly like to avoid repeating that.
> > > > I will double check this. Her Sutter's paper have this spelling and I
> > > > believe that the Microsoft implementation is following the same
> > > > spelling. The main difference is that the MSVC version does not have a
> > > > type argument at this point but they do plan to add it as an optional
> > > > argument. (They want to infer the pointee type when it is not
> > > > provided.) The clang community do not want the inference, hence we made
> > > > the type argument required. Always providing the type argument should
> > > > be compatible with future versions of the MSVC implementation.
> > > I take this to mean that we're not implementing the same attribute as
> > > MSVC is, and that worries me.
> > >
> > > > Always providing the type argument should be compatible with future
> > > > versions of the MSVC implementation.
> > >
> > > The problem is that code written for MSVC (where the type argument is not
> > > required) will not compile with Clang (where the type argument is
> > > required), if I understand properly.
> > >
> > > Generally speaking, when we implement an attribute from another vendor
> > > namespace, we expect the attribute to have the same semantics as whoever
> > > "owns" that vendor namespace. It's a bit trickier here because `gsl`
> > > isn't really a vendor so much as a collective, so I don't know who is the
> > > authority to turn to.
> > >
> > > On a completely different note: would this attribute also make sense
> > > within C with a C2x spelling?
> > I see, that is a valid concern. A followup patch makes the type argument
> > optional so it will be fully compatible with MSVC. I don't think it makes
> > sense with C or at least I am not sure what would I annotate using these
> > attributes in C code as the subjects are classes. I know that it is
> > possible to simulate classes in C but I am not sure how well the analysis
> > would work in such cases.
> >
> > Are you OK with making the argument optional in a followup patch or do you
> > want us to cherry-pick that modification into this one?
> > I don't think it makes sense with C or at least I am not sure what would I
> > annotate using these attributes in C code as the subjects are classes. I
> > know that it is possible to simulate classes in C but I am not sure how
> > well the analysis would work in such cases.
>
> I couldn't think of a reason to expose this in C either, so I think the
> current spelling is good as-is.
>
> > Are you OK with making the argument optional in a followup patch or do you
> > want us to cherry-pick that modification into this one?
>
> I'd prefer the argument be optional in this patch.
The argument is now optional.
================
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:
> 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.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:335-341
+ if (attributeIsTypeArgAttr(*AttrName)) {
+ ParseAttributeWithTypeArg(*AttrName, AttrNameLoc, Attrs, EndLoc, ScopeName,
+ ScopeLoc, Syntax);
+ // FIXME: when attributeIsTypeArgAttr() is true, assumes that the attribute
+ // takes a single parameter.
+ return 1;
+ }
----------------
aaron.ballman wrote:
> I don't like how this short-circuits the remainder of the logic in this
> function. I'd rather see this one-off logic handled in
> `ParseClangAttributeArgs()` if we have to treat it as a one-off. A better
> approach would be to implement type arguments within
> `ParseAttributeArgsCommon()`.
I integrated parsing of type attributes now into the main logic of
`ParseAttributeArgsCommon()`
================
Comment at: clang/lib/Sema/SemaDecl.cpp:2618-2619
+ } else if (isa<OwnerAttr>(NewAttribute) || isa<PointerAttr>(NewAttribute))
{
+ // gsl::Owner and gsl::Pointer are allowed to be added to a class after
it
+ // is defined.
+ ++I;
----------------
aaron.ballman wrote:
> I'd like to see some tests for this.
>
> I'm a bit worried about whether this will do good things in practice or not.
> I'm concerned about situations like:
> ```
> struct S;
>
> void func(S *s) {
> // Interesting things happen here.
> }
>
> struct [[gsl::Pointer]] S {};
> ```
> and whether the "interesting things" will be properly [un]diagnosed.
I have removed this part of the PR. Implementing implicit attributes for `std`
types went well without requiring to allow "late" additions of attributes.
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
