Page MenuHomePhabricator

[libc++] Complete overhaul of constexpr support in std::array
ClosedPublic

Authored by ldionne on Fri, May 22, 12:38 PM.

Details

Summary

This commit adds missing support for constexpr in std::array under all
standard modes up to and including C++20. It also transforms the <array>
tests to check for constexpr-friendliness under the right standard modes.

It does change one bit of behavior from the previous array implementation,
notably the fact that begin() and end() on an empty std::array will now
return nullptr. This implementation is allowed by the current wording
for std::array, however if https://wg21.link/LWG2157 goes through, we'll
have to change that strategy for one where different empty arrays have
iterators to different underlying ranges.

Fixes https://llvm.org/PR40124
Fixes rdar://57522096
Supersedes https://reviews.llvm.org/D60666

Diff Detail

Event Timeline

ldionne created this revision.Fri, May 22, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 22, 12:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a reviewer: zoecarver.EditedFri, May 22, 12:41 PM

I know there's been discussion before about having to return nullptr from begin() and end() for empty arrays and we didn't want to do it. However, doing so is a valid implementation strategy with the given Standard IIUC, and it would only become incorrect if we adopted https://wg21.link/LWG2157. Given that LWG2157 hasn't made it through yet, I think it's a lot more important to fix the situation of std::array in libc++ than implement a maybe-future LWG issue resolution.

Also note that if LWG goes through, there's presumably a way to implement it, so we can just switch to that. However, I don't think removing constexpr for the empty std::arrays is ever going to happen, because that would be way too inconsistent.

miscco added a subscriber: miscco.Fri, May 22, 1:12 PM

Thanks a lot for improving the tests. I am not yet comfortable with libc++ tests so I have some general questions inline

libcxx/test/std/containers/sequences/array/array.cons/deduct.pass.cpp
33

being new to the party, what is the reason for the template?

libcxx/test/std/containers/sequences/array/array.cons/implicit_copy.pass.cpp
39

Would it make sense to consistently drop all those typedefs?

If not what is their purpose?

libcxx/test/std/containers/sequences/array/array.cons/initializer_list.pass.cpp
26

preexisting: Are those conversions from into double intentional?

42

Same here int -> double conversion

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

This is TEST_CONSTEXPR_CXX14 but the constexpr test is #if TEST_STD_VER >= 17

Could we define a macro TEST_CONSTEXPR_CXX14?

Notably this should not compile for C++14 as data is not constexpr.

Occurs at other places too

Thanks for all the test improvements @ldionne!


What's the point of all the Dummy templates?

libcxx/include/array
368

This will probably break some people's code. I would much prefer not returning nullptr here. What's the downside of using something similar to D60666's implementation of data()?

libcxx/test/std/containers/sequences/array/array.cons/deduct.pass.cpp
33

I don't think the template is needed.

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

I don't see the point of this test anymore (lines 48-54).

libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
87

Same as above.

112

pint will always be zero, no? I think it would make more sense to test this with non-null data (std::array<T, 1> maybe).

What I completely forgot, thisimplements the array portion of the [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1032r1.html | misc_constexpr ]]paper so you should adopt/add the feature test macro `__cpp_lib_array_constexpr = 201811L``

zoecarver added inline comments.Sat, May 23, 11:05 AM
libcxx/include/array
261

Why use a new _Empty struct? char makes more sense and has a defined size in the standard.

261

Nit: this could be add_const.

ldionne updated this revision to Diff 266051.Mon, May 25, 10:52 AM
ldionne marked 20 inline comments as done.

Address review comments. Main change: an empty array begin()/end() doesn't return nullptr anymore, i.e. there is no behavior change.

Thanks @zoecarver and @miscco for the comments and review! I'm addressing your comments.

What I completely forgot, thisimplements the array portion of the [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1032r1.html | misc_constexpr ]]paper so you should adopt/add the feature test macro `__cpp_lib_array_constexpr = 201811L``

Good catch! Will add in the next update.

libcxx/include/array
261

Default constructing a char leaves it to an undefined value, which you can't do inside constexpr. So something like std::array<int, 0> a; isn't valid inside constexpr if the empty-array implementation holds a char internally, but it does work with an empty struct.

But regardless, this is being undone anyway since I'm moving to the union trick.

368

Indeed -- I spent some more time on this and it does work. Updated.

libcxx/test/std/containers/sequences/array/array.cons/deduct.pass.cpp
33

It's to avoid this error:

at.pass.cpp:27:27: error: constexpr function never produces a constant expression [-Winvalid-constexpr]
TEST_CONSTEXPR_CXX14 bool tests()

It triggers when e.g. at() is not marked as constexpr in the given standard mode, but the tests() function that calls it is.

However, in this specific case, it's not useful because the test is only enabled in c++17 and above, so there's no constexpr mismatch. But it is useful in other tests, like the ones for at, data and probably others. I'll remove the unnecessary ones.

libcxx/test/std/containers/sequences/array/array.cons/implicit_copy.pass.cpp
39

I think it would make sense to drop them -- I'm not a big fan. It's just the way the tests were written originally I guess. I don't really want to systematically remove the typedefs in this patch, though.

libcxx/test/std/containers/sequences/array/array.cons/initializer_list.pass.cpp
26

I am pretty sure they are, we're likely testing that aggregate initialization works as expected.

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

Could we define a macro TEST_CONSTEXPR_CXX14?

Do you mean TEST_CONSTEXPR_CXX17?

I'm making the test code constexpr whenever it can be marked as such, I'm just not checking the constexpr-ness of std::array itself unless the Standard mandates it. That's the purpose of the Dummy template parameter -- it makes it OK to have constexpr on the tests() function in C++14, despite its body not being constexpr-friendly (because data() isn't constexpr as you mention).

52

I've re-added the check for p != nullptr, so this is moot.

libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
112

Moot since I'm using the union trick.

miscco added inline comments.Mon, May 25, 11:59 PM
libcxx/include/array
368

I would like to note that this will break users code once they want to try different standard library implementations.

Given that the other two major implementations use nullptr as return value this is a clear portability issue. Is there any other benefit to doing so?

I would note that
a. This seems like an supremely rare case anyway
b. I would consider breaking code that relies on UB to be less or equally severe than breaking code when switching standard libraries

That said I wouldn't shed any tears if you go with that implementation

libcxx/test/std/containers/sequences/array/array.cons/deduct.pass.cpp
33

But if one chooses the "right" TEST_CONSTEXPR_CXX macro this should not happen?

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

Again, Why not simply declare this as TEST_CONSTEXPR_CXX17 and be correct in every case. Is there any benefit to not doing so?

ldionne marked 6 inline comments as done.Wed, May 27, 1:59 PM
ldionne added inline comments.
libcxx/include/array
368

The main problem is that once https://cplusplus.github.io/LWG/issue2157 gets through, we'll have to reimplement it the way this patch currently does anyway (i.e. not return nullptr). It's also technically an ABI break and a behavior change to start returning nullptr, but frankly I don't think it's an important enough one to be bothered. But I do think doing the same thing as other implementations is not something we should strive for *in this case* because of the LWG issue.

libcxx/test/std/containers/sequences/array/array.cons/deduct.pass.cpp
33

Indeed. Ok, I'll do that instead.

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

I guess there isn't much benefit after all. I liked that the standard we tested constexpr under was defined in a single place (where we call tests()), but I guess that's minor for the annoyance it causes.

ldionne updated this revision to Diff 266657.Wed, May 27, 1:59 PM
ldionne marked 3 inline comments as done.

Use the right constexpr qualifiers.

@zoecarver @miscco Please take a look and LMK if you see other problems with the patch. I'll ship it when I get your LGTMs.

ldionne updated this revision to Diff 266659.Wed, May 27, 2:04 PM

Remove forgotten dummy template.

Thanks for fixing the nullptr thing, and thanks for all the work to improve tests here. Other than my one comment, this LGTM.

libcxx/include/array
245

This should be after C++11. In C++11 this won't be an aggregate (__w is a non-static member without initialization).

269

Nit: will you put a comment here?

zoecarver added inline comments.Wed, May 27, 2:50 PM
libcxx/include/array
245

__w is a non-static member without initialization.

This should say: "__w is a non-static member with default initialization."

245

After a quick scan it looks like there is no test for is_aggregate<array<T, N>>. Any chance you could add that (note: I think it is important to have checks for both is_aggregate<array<T, N>> and is_aggregate<array<T, 0>>)?

I had two minor nits and a general OMG moment about the diverging constexpr status wrt indexing .

Besides that LGTM

libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp
83–84

Nit: In the other C++17 defines you use #if TEST_STD_VER >= 17 and not #if TEST_STD_VER > 14

84–89

Nit: This always tests compile time. It would be more consistent with the other tests to use assert here and test the constexpr behavior via the static_assert(tests(), "");

libcxx/test/std/containers/sequences/array/front_back_const.pass.cpp
25

Comment: I wrote multiple comments to check whether this is really different. than the non-const version.

Checking with the standard shows it indeed is (terrible).

This LGTM!

libcxx/include/array
245

Never mind about this. I got confused about the aggregate/default initialization requirements.

ldionne updated this revision to Diff 266915.Thu, May 28, 9:20 AM
ldionne marked 10 inline comments as done.

Add tests for is_aggregate

libcxx/include/array
245

This should be after C++11. In C++11 this won't be an aggregate (__w is a non-static member without initialization).

I don't think that's correct. I think it is an aggregate -- __w doesn't have a member initializer. Clang seems to agree with me using __is_aggregate. In all cases, I'm adding tests.

269

IMO the code block is small enough that a comment on the closing #endif isn't useful.

libcxx/test/std/containers/sequences/array/front_back_const.pass.cpp
25

Yes it is -- initially constexpr implied const on variables, which sheds some light on why things evolved that way.

ldionne accepted this revision as: Restricted Project.Thu, May 28, 9:23 AM
ldionne marked an inline comment as done.
zoecarver accepted this revision.Thu, May 28, 9:26 AM
This revision is now accepted and ready to land.Thu, May 28, 9:26 AM

Thanks all for your comments, it was really useful in improving the patch. I'll try to ship this now and we'll see if it breaks something unexpectedly.

This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedFri, May 29, 12:37 PM
#include <array>
#include <string>

int main() {
  std::array<std::string, 0> cells;
}

>

a.cc:5:30: error: call to implicitly-deleted default constructor of 'std::array<std::string, 0>' (aka 'array<basic_string<char, char_traits<char>, all
ocator<char>>, 0>')
  std::array<std::string, 0> cells;
                             ^
/tmp/ReleaseA/bin/../include/c++/v1/array:252:7: note: default constructor of 'array<std::__1::basic_string<char, std::__1::char_traits<char>, std::__
1::allocator<char>>, 0>' is implicitly deleted because field '__w' has a deleted destructor
    } __w;                    
      ^                                                                                                                                               
/tmp/ReleaseA/bin/../include/c++/v1/array:248:9: note: explicitly defaulted function was implicitly deleted here
        ~__wrapper() = default;
        ^
/tmp/ReleaseA/bin/../include/c++/v1/array:251:13: note: destructor of '__wrapper' is implicitly deleted because variant field '__t' has a non-trivial 
destructor                     
        _Tp __t;

tensorflow/core/lib/monitoring/gauge.h can have such a use case of 0 with certain instantiations.

Edit: just saw D80821. Thanks for the patch.

Hi! I think this patch might be the cause of the bug at https://bugs.llvm.org/show_bug.cgi?id=46137.

Essentially, we get an error when attempting to create std::array<S, 0> if S has a non-trivial destructor (full example at the bug link):

test.cc:20:20: error: call to implicitly-deleted default constructor of 'std::array<S, 0>'
  std::array<S, 0> x;  // bad
                   ^
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/array:252:7: note: default constructor of 'array<S, 0>' is implicitly deleted because field '__w' has a deleted destructor
    } __w;
      ^
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/array:248:9: note: explicitly defaulted function was implicitly deleted here
        ~__wrapper() = default;
        ^
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/array:251:13: note: destructor of '__wrapper' is implicitly deleted because variant field '__t' has a non-trivial destructor
        _Tp __t;
            ^
test.cc:20:20: error: attempt to use a deleted function
  std::array<S, 0> x;  // bad
                   ^

I believe std::arrays of size 0 should be legal regardless of constraints on the element type.

Hi! I think this patch might be the cause of the bug at https://bugs.llvm.org/show_bug.cgi?id=46137.

Essentially, we get an error when attempting to create std::array<S, 0> if S has a non-trivial destructor (full example at the bug link):

test.cc:20:20: error: call to implicitly-deleted default constructor of 'std::array<S, 0>'
  std::array<S, 0> x;  // bad
                   ^
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/array:252:7: note: default constructor of 'array<S, 0>' is implicitly deleted because field '__w' has a deleted destructor
    } __w;
      ^
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/array:248:9: note: explicitly defaulted function was implicitly deleted here
        ~__wrapper() = default;
        ^
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-1-master-fuchsia-toolchain/install/bin/../include/c++/v1/array:251:13: note: destructor of '__wrapper' is implicitly deleted because variant field '__t' has a non-trivial destructor
        _Tp __t;
            ^
test.cc:20:20: error: attempt to use a deleted function
  std::array<S, 0> x;  // bad
                   ^

I believe std::arrays of size 0 should be legal regardless of constraints on the element type.

Woops. Sorry. Disregard my comment. Didn't refresh and see @MaskRay's comment or D80821. Thanks for the fix!