Page MenuHomePhabricator

[P0936R0] add [[clang::lifetimebound]] attribute
ClosedPublic

Authored by rsmith on Jul 27 2018, 11:06 AM.

Details

Summary

This patch adds support for a new attribute, [[clang::lifetimebound]], that indicates that the lifetime of a function result is related to one of the function arguments. When walking an initializer to make sure that the lifetime of the initial value is at least as long as the lifetime of the initialized object, we step through parameters (including the implicit object parameter of a non-static member function) that are marked with this attribute.

There's nowhere to write an attribute on the implicit object parameter, so in lieu of that, it may be applied to a function type (where it appears immediately after the cv-qualifiers and ref-qualifier, which is as close to a declaration of the implicit object parameter as we have). I'm currently modeling this in the AST as the attribute appertaining to a ParmVarDecl or to a function type, but in the latter case I'd be happy to stash it somewhere else if there is a better home for it.

Diff Detail

Repository
rC Clang

Event Timeline

rsmith created this revision.Jul 27 2018, 11:06 AM
rsmith added inline comments.Jul 27 2018, 11:10 AM
include/clang/AST/TypeLoc.h
96–97 ↗(On Diff #157711)

(This typo fix has been committed separately.)

include/clang/Basic/AttrDocs.td
2368

I've removed the underscore here.

lib/AST/TypePrinter.cpp
1491–1492

(This was dead code; see lines 1396-1399.)

aaron.ballman added inline comments.Jul 28 2018, 10:28 AM
include/clang/Basic/AttrDocs.td
2371

I read this as allowing a lifetimebound attribute on a constructor, but that seems to be an error case.

lib/AST/TypePrinter.cpp
1491–1492

Good catch! Do you mind committing this separately?

lib/Parse/ParseDeclCXX.cpp
3811 ↗(On Diff #157711)

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.

lib/Sema/SemaDecl.cpp
6013

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.

6018

This comment doesn't match reality regarding ctors.

6022–6028

Can elide the braces.

lib/Sema/SemaInit.cpp
6370

const FunctionDecl * (and threaded through).

lib/Sema/SemaType.cpp
7202–7207

Elide braces

rsmith updated this revision to Diff 158149.Jul 30 2018, 6:02 PM
rsmith marked 8 inline comments as done.
rsmith added inline comments.
include/clang/Basic/AttrDocs.td
2371

A lifetimebound attribute is permitted on a parameter of a constructor. I've reworded to clarify.

lib/Parse/ParseDeclCXX.cpp
3811 ↗(On Diff #157711)

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 :)

lib/Sema/SemaDecl.cpp
6013

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).

6022–6028

True, but the controlled statements are multi-line, so I'd prefer not to. :)

lib/Sema/SemaOverload.cpp
5233–5235 ↗(On Diff #157711)

Reverted this change; I committed this separately.

lib/Sema/SemaType.cpp
7202–7207

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.

aaron.ballman accepted this revision.Jul 31 2018, 1:59 PM

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.

lib/Sema/SemaDecl.cpp
6013

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!

6022–6028

Sold.

lib/Sema/SemaInit.cpp
6959

Is this something you intended to handle in this patch and forgot, or are you saving this for a future patch?

lib/Sema/SemaType.cpp
7202–7207

Sounds good.

test/SemaCXX/attr-lifetimebound.cpp
4

Can you also add a test demonstrating the attribute doesn't accept any arguments?

5

Something you intended to support in this patch?

103

Agreed; that is a bit ugly.

This revision is now accepted and ready to land.Jul 31 2018, 1:59 PM
rsmith marked 3 inline comments as done.Jul 31 2018, 5:32 PM
rsmith added inline comments.
lib/Sema/SemaInit.cpp
6959

It's something I'm going to experiment with and see if it improves the diagnostic quality, but not for this patch.

test/SemaCXX/attr-lifetimebound.cpp
5

More like an open question for the standards proposal. I've slightly reworded the FIXME to indicate that this is an open question.

This revision was automatically updated to reflect the committed changes.
rsmith marked 2 inline comments as done.
kimgr added a subscriber: kimgr.Aug 1 2018, 11:11 PM

Potential typo in the tests

test/SemaCXX/attr-lifetimebound.cpp
24

Should this say 'non-ref' instead of 'non-void'?

This change made clang to start trigger failed asserts for me (although they seem to not be reproducible with all compilers building clang), see https://bugs.llvm.org/show_bug.cgi?id=38421 for full description.