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 ¶m [[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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
