[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2770
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[CXXRecord]>;
----------------
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?


================
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;
+  }
----------------
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()`.


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954