[PATCH] D63954: Add lifetime categories attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D63954#1564448 <https://reviews.llvm.org/D63954#1564448>, @gribozavr wrote:

> > We explicitly allow to add an annotation after the definition of the class to allow adding annotations to external source from by the user
>
> Asking users to forward-declare anything from the standard library is a very bad idea -- in fact it is undefined behavior. https://stackoverflow.com/questions/307343/forward-declare-an-stl-container
>
> The compiler should just know about standard library types and attach attributes automatically.


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?

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?



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


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4564
+      if (Attr->getDerefType().getTypePtr() != ParmType.getTypePtr()) {
+        S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible) << AL << Attr;
+        S.Diag(Attr->getLocation(), diag::note_conflicting_attribute);
----------------
erik.pilkington wrote:
> `diag::warn_duplicate_attr`?
We actually allow duplicated attributes as long as they are not conflicting. The reason why we introduced a new diagnostic is that the source of the problem is the conflict, i.e.: the two attributes are contradicting each other. Not the duplication.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4537
+static void handleLifetimeCategoryAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!AL.hasParsedType()) {
+    S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << AL << 1;
----------------
aaron.ballman wrote:
> Under what circumstances is this check needed? I believe this is already automatically handled for you.
It is handled automatically indeed, thanks!


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+    S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << AL;
+    return;
----------------
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`.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4553
+  // we always add (and check) the attribute to the cannonical decl.
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {
----------------
aaron.ballman wrote:
> Will this work? What happens if we see a forward declaration with this attribute first and then see the canonical declaration later? I suspect this work needs to be done when merging declaration attributes instead.
For `TagDecl`s the canonical decl is the first declaration:
```
TagDecl *TagDecl::getCanonicalDecl() { return getFirstDecl(); }
```

So I suspect we can never see a non-canonical declaration first. And once we see the canonical declaration it remains the same no matter how many new declaration do we see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954