Page MenuHomePhabricator

[libc++] Fix issues with the triviality of std::array
ClosedPublic

Authored by ldionne on May 29 2020, 10:49 AM.

Details

Summary

The Standard is currently unimplementable. We have to pick between:

  1. Not implementing constexpr support properly in std::array<T, 0>
  2. Making std::array<T, 0> non-trivial even when T is trivial
  3. Returning nullptr from std::array<T, 0>::begin()

Libc++ initially picked (1). In 77b9abfc8e89, we started implementing constexpr properly, but lost the guarantee of triviality. Since it seems like both (1) and (2) are really important, it seems like (3) is the only viable option for libc++, after all. This is also what other implementations are doing.

This patch moves libc++ from (1) to (3).

It also:

  • Improves the test coverage for the various ways of initializing std::array
  • Adds tests for the triviality of std::array
  • Adds tests for the aggregate-ness of std::array

Diff Detail

Event Timeline

ldionne created this revision.May 29 2020, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 10:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@ldionne I disagree. I think we can have all the things here :)

We're going to need four overloads in order to make this work (hopefully factored out into their own helper object):

  1. N != 0: use the current implementation (or something equivalent).
  2. T is trivially copyable *and* we're in a constexpr context: hold a char array and use bit_cast to convert it to T*.
  3. T is trivially copyable and where *not* in a constexpr context: hold a char array and use reinterpret_cast (to eliminate an extra stack alloc).
  4. T is not trivially copyable: use the union trick (currently implemented in master).

I'm happy to help implement this if you want (I know you've already put a lot of time into improving std::array so, I'm happy to help wherever I can).

@ldionne I disagree. I think we can have all the things here :)

We're going to need four overloads in order to make this work (hopefully factored out into their own helper object):

  1. N != 0: use the current implementation (or something equivalent).
  2. T is trivially copyable *and* we're in a constexpr context: hold a char array and use bit_cast to convert it to T*.

We can't use bit_cast for converting pointer types like that. From cppreference / bit_cast:

This function template is constexpr if and only if each of To, From and the types of all subobjects of To and From:

  • is not a union type;
  • is not a pointer type;
  • is not a pointer to member type;
  • is not a volatile-qualified type; and
  • has no non-static data member of reference type.

I really don't think there's a way around this one, as it would need to relax the rules of the constexpr interpreter to allow synthetizing a pointer to a T from a pointer to arbitrary memory. We specifically don't allow that.

ldionne updated this revision to Diff 267328.May 29 2020, 12:07 PM

Make independent from https://reviews.llvm.org/D80790 and merge the tests from https://reviews.llvm.org/D80820.

This can now be reviewed on its own.

ldionne retitled this revision from [libc++] Return nullptr from an empty array begin() to [libc++] Fix issues with the triviality of std::array.May 29 2020, 12:09 PM
ldionne edited the summary of this revision. (Show Details)
miscco added a subscriber: miscco.May 29 2020, 12:17 PM

I think this is the "least worse" solution to the mess that is array<T, 0>.

Adding additional implementation complexity for something the is impossible to actually use and only remotely relevant for meta programming is IMHO hardly justified.

miscco accepted this revision.May 29 2020, 12:45 PM
EricWF accepted this revision.May 29 2020, 12:55 PM
EricWF added a subscriber: EricWF.
EricWF added inline comments.
libcxx/test/std/containers/sequences/array/array.cons/implicit_copy.pass.cpp
75

These tests aren't changing in behavior, right? You've just re-formated and renamed them?

libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp
52

I'm OK changing this to p == nullptr if that's the behavior we all implement.

This revision is now accepted and ready to land.May 29 2020, 12:55 PM
zoecarver accepted this revision.May 29 2020, 1:07 PM

@ldionne Apologies that I just committed the patch on your behalf. I need this fix to make internal tensorflow builds work https://reviews.llvm.org/D80452#2063918
See https://reviews.llvm.org/D80452#2064445 for another independent report.

This revision was automatically updated to reflect the committed changes.
ldionne marked 4 inline comments as done.May 30 2020, 5:46 AM
ldionne added inline comments.
libcxx/test/std/containers/sequences/array/array.cons/implicit_copy.pass.cpp
75

Yes, that is correct.

libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp
52

Will do in a follow up patch.

ldionne marked 2 inline comments as done.May 30 2020, 5:48 AM

@ldionne Apologies that I just committed the patch on your behalf. I need this fix to make internal tensorflow builds work https://reviews.llvm.org/D80452#2063918
See https://reviews.llvm.org/D80452#2064445 for another independent report.

No worries, thanks for committing it. I just saw the thread this morning and was going to commit it in a hurry, but you've already done so. Thanks!