[PATCH] D63954: Add lifetime categories attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63954#1562928 <https://reviews.llvm.org/D63954#1562928>, @erik.pilkington 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, e.g.
> > 
> >   #include <vector>
> >   
> >   namespace std {
> >   template<typename T, typename Alloc>
> >   class [[gsl::Owner(T)]] vector;
> >   }
>
> Wait, does that even work? What if the vector was declared in an inline namespace in the header? Seems like a strange recommendation for users. Is there some reason you can't just add these annotations to standard libraries? I doubt libcxx would have a problem with getting better warnings on their types.


We do not want to encourage users to redeclare things within namespace std like this -- I believe that is UB.



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


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4167
+pointer/reference to the data owned by ``O``. The owned data is assumed to end
+its lifetime once the owning object's lifetime end.
+
----------------
the owning object's lifetime end -> the owning object's lifetime ends


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4177
+  let Content = [{
+When annotating a class ``P`` with ``[[gsl::Pointer(T)]]``, it assumed to be a
+non-owning type whose objects can point to ``T`` type objects or dangle.
----------------
You don't reference `P` in the rest of the docs, is that intentional?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4186
+}
\ No newline at end of file

----------------
Please keep the newline at the end of the file.


================
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;
----------------
Under what circumstances is this check needed? I believe this is already automatically handled for you.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4546-4547
+
+  if (ParmType->isVoidType()) {
+    S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << "'void'" << AL;
+    return;
----------------
Is `void` really the only problematic type? What about incomplete types, for instance?


================
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) {
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4554
+  D = D->getCanonicalDecl();
+  if(AL.getKind() ==  ParsedAttr::AT_Owner) {
+    if (checkAttrMutualExclusion<PointerAttr>(S, D, AL))
----------------
Formatting.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4557
+      return;
+    if (const auto *Attr = D->getAttr<OwnerAttr>()) {
+      if (Attr->getDerefType().getTypePtr() != ParmType.getTypePtr()) {
----------------
Please use a name other than `Attr` (as that's a type name).


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:61
+class [[gsl::Owner(int)]] AddTheSameLater;
\ No newline at end of file

----------------
Please add a newline to the end of the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954