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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith 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++.
----------------
aaron.ballman wrote:
> I read this as allowing a `lifetimebound` attribute on a constructor, but that seems to be an error case.
A `lifetimebound` attribute is permitted on a parameter of a constructor.  I've reworded to clarify.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3811
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_LifetimeBound:
     return true;
----------------
aaron.ballman wrote:
> 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.
This also controls whether redundant parens are permitted, which is what I wanted to disallow.

However, some testing of this has revealed that I actually do want an optional argument, so I'll leave the checking out for now, and we can talk about that in the next patch :)


================
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();
----------------
aaron.ballman wrote:
> `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.
Done. I don't think it makes any difference, though: this constness is shallow, and doesn't defend against us somehow accidentally mutating the `TypeSourceInfo` (which is all we're looking at, and `TypeLoc` doesn't have a `const` view).


================
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();
+        }
----------------
aaron.ballman wrote:
> Can elide the braces.
True, but the controlled statements are multi-line, so I'd prefer not to. :)


================
Comment at: lib/Sema/SemaOverload.cpp:5233-5235
+  if (From->getType()->isPointerType())
+    return From;
+  return TemporaryMaterializationConversion(From);
----------------
Reverted this change; I committed this separately.


================
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);
+  }
----------------
aaron.ballman wrote:
> Elide braces
I'd prefer to not elide braces around a multi-line `if` body, and I'd strongly prefer to avoid bracing an `if` and not its `else`.


https://reviews.llvm.org/D49922