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