This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Make std::tuple<> trivially constructible
ClosedPublic

Authored by ldionne on May 29 2019, 12:12 PM.

Details

Summary

This is not mandated by the Standard, but it's nonetheless a nice
property to have, especially since it's so easy to implement. It
also shrinks our bug list!

PR41714

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.May 29 2019, 12:12 PM

Other than the "this is not standard behavior", and my general feeling about "things that everyone does, but aren't in the standard", I'm fine with this.
Once we get an LWG issue open on this behavior, I'll be strongly in favor of this.

I have no problems with the code, btw.

I opened a LWG issue per your request, I'm waiting for the LWG chair (hmm, I wonder who that is) to give me an issue number 🙃.

Changing the triviality of a type can be ABI breaking because it will change the calling conventions the compiler uses.
We have hacks in pair to keep it non-trivial on some platforms for this reason.

I'm not sure if this case triggers that change (it may only be when you change is_trivially_copyable). But we need to double check.

libcxx/include/tuple
910 ↗(On Diff #202018)

This is C++11 and up so you can just write = default.

Changing the triviality of a type can be ABI breaking because it will change the calling conventions the compiler uses.
We have hacks in pair to keep it non-trivial on some platforms for this reason.

Right, I think we at least need a macro to choose the ABI so that (a) vendors can pick a default and (b) users can override it.

ldionne marked an inline comment as done.May 29 2019, 1:34 PM

Changing the triviality of a type can be ABI breaking because it will change the calling conventions the compiler uses.
We have hacks in pair to keep it non-trivial on some platforms for this reason.

I'm not sure if this case triggers that change (it may only be when you change is_trivially_copyable). But we need to double check.

I was going to close PR41714 as "Can't fix, this is an ABI break", but then I checked the Itanium C++ ABI and it did not mention default constructors. Here, it says:

*non-trivial for the purposes of calls*
A type is considered non-trivial for the purposes of calls if:

  • it has a non-trivial copy constructor, move constructor, or destructor, or
  • all of its copy and move constructors are deleted.

This definition, as applied to class types, is intended to be the complement of the definition in [class.temporary]p3 of types for which an extra temporary is allowed when passing or returning a type. A type which is trivial for the purposes of the ABI will be passed and returned according to the rules of the underlying C ABI, e.g. in registers; often this has the effect of performing a trivial copy of the type.

I'd welcome a double-check, but I think this isn't ABI breaking. Otherwise the resolution is evident, y'all know me :-).

libcxx/include/tuple
910 ↗(On Diff #202018)

I thought this was trying to work in C++03 -- isn't that why we're using _NOEXCEPT and _LIBCPP_CONSTEXPR??

EricWF accepted this revision.May 29 2019, 1:51 PM

Thanks for the link Louis. This should be safe then. .

libcxx/include/tuple
910 ↗(On Diff #202018)

Not really. And even then I believe Clang accepts = default as an extension in C++03.

This revision is now accepted and ready to land.May 29 2019, 1:51 PM
mclow.lists added inline comments.May 29 2019, 2:52 PM
libcxx/include/tuple
910 ↗(On Diff #202018)

Yes.

ldionne updated this revision to Diff 202236.May 30 2019, 10:33 AM
ldionne marked 2 inline comments as done.

Use = default per Eric's comment.

Just to confirm, @mclow.lists are we OK with this since the LWG issue has been filed?

mclow.lists accepted this revision.Jun 5 2019, 8:56 AM

Just to confirm, @mclow.lists are we OK with this since the LWG issue has been filed?

Yes - thanks for checking.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 7:59 AM