[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/AttrDocs.td:2371
+is retained by the return value of the annotated function
+(or, for a constructor, in the value of the constructed object).
+It is only supported in C++.
----------------
I read this as allowing a `lifetimebound` attribute on a constructor, but that seems to be an error case.


================
Comment at: lib/AST/TypePrinter.cpp:1492-1521
-  case AttributedType::attr_objc_gc: {
-    OS << "objc_gc(";
-
-    QualType tmp = T->getEquivalentType();
-    while (tmp.getObjCGCAttr() == Qualifiers::GCNone) {
-      QualType next = tmp->getPointeeType();
-      if (next == tmp) break;
----------------
rsmith wrote:
> (This was dead code; see lines 1396-1399.)
Good catch! Do you mind committing this separately?


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3811
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_LifetimeBound:
     return true;
----------------
Hmmm, this isn't a standards-based attribute yet. The only thing this controls, however, is not parsing a duplicate  attribute within the same attribute specifier sequence as the standard is the only thing persnickety enough to feel that warrants an error.

My personal preference is to not disallow that, especially given that users can write `void f(<blah>) [[clang::lifetimebound]][[clang::lifetimebound]]` and get no diagnostic.


================
Comment at: lib/Sema/SemaDecl.cpp:6013
+  // Check the attributes on the function type, if any.
+  if (auto *FD = dyn_cast<FunctionDecl>(&ND)) {
+    for (TypeLoc TL = FD->getTypeSourceInfo()->getTypeLoc();
----------------
`const auto *` (and thread the const-correctness through).

Actually, if you could double-check on the const correctness elsewhere in the patch, that'd be useful, as it looks like there are other places that could be similarly touched up.


================
Comment at: lib/Sema/SemaDecl.cpp:6018
+      // The [[lifetimebound]] attribute can be applied to the implicit object
+      // parameter of a non-static member function (other than a dtor) by
+      // applying it to the function type.
----------------
This comment doesn't match reality regarding ctors.


================
Comment at: lib/Sema/SemaDecl.cpp:6022-6028
+        if (!MD || MD->isStatic()) {
+          S.Diag(ATL.getAttrNameLoc(), diag::err_lifetimebound_no_object_param)
+              << !MD << ATL.getLocalSourceRange();
+        } else if (isa<CXXConstructorDecl>(MD) || isa<CXXDestructorDecl>(MD)) {
+          S.Diag(ATL.getAttrNameLoc(), diag::err_lifetimebound_ctor_dtor)
+              << isa<CXXDestructorDecl>(MD) << ATL.getLocalSourceRange();
+        }
----------------
Can elide the braces.


================
Comment at: lib/Sema/SemaInit.cpp:6369
+
+static bool implicitObjectParamIsLifetimeBound(FunctionDecl *FD) {
+  TypeSourceInfo *TSI = FD->getTypeSourceInfo();
----------------
`const FunctionDecl *` (and threaded through).


================
Comment at: lib/Sema/SemaType.cpp:7202-7207
+  if (D.isDeclarationOfFunction()) {
+    CurType = S.Context.getAttributedType(AttributedType::attr_lifetimebound,
+                                          CurType, CurType);
+  } else {
+    Attr.diagnoseAppertainsTo(S, nullptr);
+  }
----------------
Elide braces


Repository:
  rC Clang

https://reviews.llvm.org/D49922