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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, though I raised a few questions. If you want to handle them as part of this patch, I'm happy to do another round of review. If you want to handle them in a follow-up patch, I'm also okay.



================
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();
----------------
rsmith wrote:
> 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).
Yeah, our const-correctness story is pretty hit or miss. However, I try to ensure new code attempts to be const-correct if it's not too annoying because I'm still a hopeful, optimistic kind of guy. :-P Thanks for handling this!


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


================
Comment at: lib/Sema/SemaInit.cpp:6959
+      case IndirectLocalPathEntry::LifetimeBoundCall:
+        // FIXME: Consider adding a note for this.
+        break;
----------------
Is this something you intended to handle in this patch and forgot, or are you saving this for a future patch?


================
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);
+  }
----------------
rsmith wrote:
> 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`.
Sounds good.


================
Comment at: test/SemaCXX/attr-lifetimebound.cpp:3
+
+namespace usage_invalid {
+  // FIXME: Diagnose void return type?
----------------
Can you also add a test demonstrating the attribute doesn't accept any arguments?


================
Comment at: test/SemaCXX/attr-lifetimebound.cpp:4
+namespace usage_invalid {
+  // FIXME: Diagnose void return type?
+  void voidreturn(int &param [[clang::lifetimebound]]);
----------------
Something you intended to support in this patch?


================
Comment at: test/SemaCXX/attr-lifetimebound.cpp:102
+  void use_reversed_range() {
+    // FIXME: Don't expose the name of the internal range variable.
+    for (auto x : reversed(make_vector())) {} // expected-warning {{temporary bound to local reference '__range1'}}
----------------
Agreed; that is a bit ugly.


https://reviews.llvm.org/D49922