[libc++] Fix constexpr initialization of std::array<T, 0> #74667
Conversation
This patch fixes constexpr default initialization of empty arrays and improves the tests accordingly. Fixes llvm#74375
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThis patch fixes constexpr default initialization of empty arrays and improves the tests accordingly. Fixes #74375 Full diff: https://github.com/llvm/llvm-project/pull/74667.diff 4 Files Affected:
diff --git a/libcxx/include/array b/libcxx/include/array
index 127092f6bca9b..334fa747339ba 100644
--- a/libcxx/include/array
+++ b/libcxx/include/array
@@ -280,7 +280,8 @@ struct _LIBCPP_TEMPLATE_VIS array<_Tp, 0>
typedef std::reverse_iterator<iterator> reverse_iterator;
typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
- typedef __conditional_t<is_const<_Tp>::value, const char, char> _CharType;
+ struct _EmptyAggregate { };
+ typedef __conditional_t<is_const<_Tp>::value, const _EmptyAggregate, _EmptyAggregate> _CharType;
struct _ArrayInStructT { _Tp __data_[1]; };
_ALIGNAS_TYPE(_ArrayInStructT) _CharType __elems_[sizeof(_ArrayInStructT)];
diff --git a/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp b/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
index 9153106b384fc..bdc79e22013f8 100644
--- a/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
+++ b/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
@@ -19,11 +19,9 @@ struct NoDefault {
};
// Test default initialization
-// This one isn't constexpr because omitting to initialize fundamental types
-// isn't valid in a constexpr context.
struct test_default_initialization {
template <typename T>
- void operator()() const
+ TEST_CONSTEXPR_CXX14 void operator()() const
{
std::array<T, 0> a0; (void)a0;
std::array<T, 1> a1; (void)a1;
@@ -34,6 +32,20 @@ struct test_default_initialization {
}
};
+// Additional tests for constexpr default initialization of empty arrays since
+// it seems that making a variable constexpr and merely having a non-constexpr
+// variable in a constexpr function behave differently.
+//
+// Reproducer for https://github.com/llvm/llvm-project/issues/74375
+struct test_default_initialization_74375 {
+ template <typename T>
+ TEST_CONSTEXPR_CXX14 void operator()() const
+ {
+ constexpr std::array<T, 0> a0; (void)a0;
+ constexpr std::array<NoDefault, 0> nodefault; (void)nodefault;
+ }
+};
+
struct test_nondefault_initialization {
template <typename T>
TEST_CONSTEXPR_CXX14 void operator()() const
@@ -177,10 +189,15 @@ TEST_CONSTEXPR_CXX14 bool with_all_types()
int main(int, char**)
{
with_all_types<test_nondefault_initialization>();
- with_all_types<test_default_initialization>(); // not constexpr
+ with_all_types<test_default_initialization>();
+ with_all_types<test_default_initialization_74375>();
test_initializer_list();
#if TEST_STD_VER >= 14
static_assert(with_all_types<test_nondefault_initialization>(), "");
+#if TEST_STD_VER >= 20
+ static_assert(with_all_types<test_default_initialization>(), "");
+#endif
+ static_assert(with_all_types<test_default_initialization_74375>(), "");
static_assert(test_initializer_list(), "");
#endif
diff --git a/libcxx/test/std/containers/sequences/array/size_and_alignment.compile.pass.cpp b/libcxx/test/std/containers/sequences/array/size_and_alignment.compile.pass.cpp
new file mode 100644
index 0000000000000..66069715103fd
--- /dev/null
+++ b/libcxx/test/std/containers/sequences/array/size_and_alignment.compile.pass.cpp
@@ -0,0 +1,130 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <array>
+
+// template <class T, size_t N>
+// struct array
+
+// Make sure std::array<T, N> has the correct object size and alignment.
+// This test is mostly meant to catch subtle ABI-breaking regressions.
+
+// Ignore error about requesting a large alignment not being ABI compatible with older AIX systems.
+#if defined(_AIX)
+# pragma clang diagnostic ignored "-Waix-compat"
+#endif
+
+#include <array>
+#include <cstddef>
+
+#include "test_macros.h"
+
+template <class T, std::size_t Size>
+struct MyArray {
+ T elems[Size];
+};
+
+template <class T>
+void test_type() {
+ {
+ static_assert(sizeof(std::array<T, 0>) == sizeof(T), "");
+ static_assert(alignof(std::array<T, 0>) == alignof(T), "");
+ static_assert(sizeof(std::array<T, 0>) == sizeof(T[1]), "");
+ static_assert(sizeof(std::array<T, 0>) == sizeof(MyArray<T, 1>), "");
+ static_assert(alignof(std::array<T, 0>) == alignof(MyArray<T, 1>), "");
+ }
+
+ {
+ static_assert(sizeof(std::array<T, 1>) == sizeof(T), "");
+ static_assert(alignof(std::array<T, 1>) == alignof(T), "");
+ static_assert(sizeof(std::array<T, 1>) == sizeof(T[1]), "");
+ static_assert(sizeof(std::array<T, 1>) == sizeof(MyArray<T, 1>), "");
+ static_assert(alignof(std::array<T, 1>) == alignof(MyArray<T, 1>), "");
+ }
+
+ {
+ static_assert(sizeof(std::array<T, 2>) == sizeof(T) * 2, "");
+ static_assert(alignof(std::array<T, 2>) == alignof(T), "");
+ static_assert(sizeof(std::array<T, 2>) == sizeof(T[2]), "");
+ static_assert(sizeof(std::array<T, 2>) == sizeof(MyArray<T, 2>), "");
+ static_assert(alignof(std::array<T, 2>) == alignof(MyArray<T, 2>), "");
+ }
+
+ {
+ static_assert(sizeof(std::array<T, 3>) == sizeof(T) * 3, "");
+ static_assert(alignof(std::array<T, 3>) == alignof(T), "");
+ static_assert(sizeof(std::array<T, 3>) == sizeof(T[3]), "");
+ static_assert(sizeof(std::array<T, 3>) == sizeof(MyArray<T, 3>), "");
+ static_assert(alignof(std::array<T, 3>) == alignof(MyArray<T, 3>), "");
+ }
+
+ {
+ static_assert(sizeof(std::array<T, 444>) == sizeof(T) * 444, "");
+ static_assert(alignof(std::array<T, 444>) == alignof(T), "");
+ static_assert(sizeof(std::array<T, 444>) == sizeof(T[444]), "");
+ static_assert(sizeof(std::array<T, 444>) == sizeof(MyArray<T, 444>), "");
+ static_assert(alignof(std::array<T, 444>) == alignof(MyArray<T, 444>), "");
+ }
+}
+
+struct Empty {};
+
+struct Aggregate {
+ int i;
+};
+
+struct WithPadding {
+ long double ld;
+ char c;
+};
+
+#if TEST_STD_VER >= 11
+struct alignas(alignof(std::max_align_t) * 2) Overaligned1 {};
+
+struct alignas(alignof(std::max_align_t) * 2) Overaligned2 {
+ char data[1000];
+};
+
+struct alignas(alignof(std::max_align_t)) Overaligned3 {
+ char data[1000];
+};
+
+struct alignas(8) Overaligned4 {
+ char c;
+};
+
+struct alignas(8) Overaligned5 {};
+#endif
+
+int main(int, char**) {
+ test_type<char>();
+ test_type<short>();
+ test_type<int>();
+ test_type<long>();
+ test_type<long long>();
+ test_type<float>();
+ test_type<double>();
+ test_type<long double>();
+ test_type<char[1]>();
+ test_type<char[2]>();
+ test_type<char[3]>();
+ test_type<Empty>();
+ test_type<Aggregate>();
+ test_type<WithPadding>();
+
+#if TEST_STD_VER >= 11
+ test_type<std::max_align_t>();
+ test_type<Overaligned1>();
+ test_type<Overaligned2>();
+ test_type<Overaligned3>();
+ test_type<Overaligned4>();
+ test_type<Overaligned5>();
+#endif
+
+ return 0;
+}
diff --git a/libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp b/libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
deleted file mode 100644
index 6fbc844a11eac..0000000000000
--- a/libcxx/test/std/containers/sequences/array/size_and_alignment.pass.cpp
+++ /dev/null
@@ -1,78 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// <array>
-
-// template <class T, size_t N>
-// struct array
-
-// Test the size and alignment matches that of an array of a given type.
-
-// Ignore error about requesting a large alignment not being ABI compatible with older AIX systems.
-#if defined(_AIX)
-# pragma clang diagnostic ignored "-Waix-compat"
-#endif
-
-#include <array>
-#include <iterator>
-#include <type_traits>
-#include <cstddef>
-
-#include "test_macros.h"
-
-template <class T, std::size_t Size>
-struct MyArray {
- T elems[Size];
-};
-
-template <class T, std::size_t Size>
-void test() {
- typedef T CArrayT[Size == 0 ? 1 : Size];
- typedef std::array<T, Size> ArrayT;
- typedef MyArray<T, Size == 0 ? 1 : Size> MyArrayT;
- static_assert(sizeof(ArrayT) == sizeof(CArrayT), "");
- static_assert(sizeof(ArrayT) == sizeof(MyArrayT), "");
- static_assert(TEST_ALIGNOF(ArrayT) == TEST_ALIGNOF(MyArrayT), "");
-}
-
-template <class T>
-void test_type() {
- test<T, 1>();
- test<T, 42>();
- test<T, 0>();
-}
-
-#if TEST_STD_VER >= 11
-struct alignas(alignof(std::max_align_t) * 2) TestType1 {
-
-};
-
-struct alignas(alignof(std::max_align_t) * 2) TestType2 {
- char data[1000];
-};
-
-struct alignas(alignof(std::max_align_t)) TestType3 {
- char data[1000];
-};
-#endif
-
-int main(int, char**) {
- test_type<char>();
- test_type<int>();
- test_type<double>();
- test_type<long double>();
-
-#if TEST_STD_VER >= 11
- test_type<std::max_align_t>();
- test_type<TestType1>();
- test_type<TestType2>();
- test_type<TestType3>();
-#endif
-
- return 0;
-}
|
You can test this locally with the following command:git-clang-format --diff 1f283a60a4bb896fa2d37ce00a3018924be82b9f 61b538dcf3da75b52e80767b569e37b798028e2b -- libcxx/test/std/containers/sequences/array/size_and_alignment.compile.pass.cpp libcxx/include/array libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp libcxx/test/std/containers/views/mdspan/extents/CtorTestCombinations.hView the diff from clang-format here.diff --git a/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp b/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp
index 786c57ebfd..4ef44545e4 100644
--- a/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp
+++ b/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp
@@ -199,7 +199,7 @@ void test()
test_not_const<void(Empty::*)(const int&) noexcept(false)>();
// Sequence containers
- test_true <std::array< int, 0>>();
+ test_true<std::array< int, 0>>();
test_not_const<std::array< int, 1>>();
test_false <std::array<const int, 1>>();
test_not_const<std::array< volatile int, 1>>();
diff --git a/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp b/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
index 7991d4738d..bbc1b3346c 100644
--- a/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
+++ b/libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
@@ -19,56 +19,73 @@ struct NoDefault {
};
struct test_initialization {
- template <typename T>
- TEST_CONSTEXPR_CXX14 void operator()() const
+ template <typename T>
+ TEST_CONSTEXPR_CXX14 void operator()() const {
+ // Check default initalization
{
- // Check default initalization
- {
- std::array<T, 0> a0; (void)a0;
- // Before C++20, default initialization doesn't work inside constexpr for
- // trivially default constructible types. This only apply to non-empty arrays,
- // since empty arrays don't hold an element of type T.
- if (TEST_STD_AT_LEAST_20_OR_RUNTIME_EVALUATED || !std::is_trivially_default_constructible<T>::value) {
- std::array<T, 1> a1; (void)a1;
- std::array<T, 2> a2; (void)a2;
- std::array<T, 3> a3; (void)a3;
- }
-
- std::array<NoDefault, 0> nodefault; (void)nodefault;
- }
-
- // A const empty array can also be default-initialized regardless of the type
- // it contains. For non-empty arrays, this doesn't work whenever T doesn't
- // have a user-provided default constructor.
- {
- const std::array<T, 0> a0; (void)a0;
- const std::array<NoDefault, 0> nodefault; (void)nodefault;
- }
+ std::array<T, 0> a0;
+ (void)a0;
+ // Before C++20, default initialization doesn't work inside constexpr for
+ // trivially default constructible types. This only apply to non-empty arrays,
+ // since empty arrays don't hold an element of type T.
+ if (TEST_STD_AT_LEAST_20_OR_RUNTIME_EVALUATED || !std::is_trivially_default_constructible<T>::value) {
+ std::array<T, 1> a1;
+ (void)a1;
+ std::array<T, 2> a2;
+ (void)a2;
+ std::array<T, 3> a3;
+ (void)a3;
+ }
+
+ std::array<NoDefault, 0> nodefault;
+ (void)nodefault;
+ }
- // Check direct-list-initialization syntax (introduced in C++11)
- #if TEST_STD_VER >= 11
- {
- {
- std::array<T, 0> a0_0{}; (void)a0_0;
- }
- {
- std::array<T, 1> a1_0{}; (void)a1_0;
- std::array<T, 1> a1_1{T()}; (void)a1_1;
- }
- {
- std::array<T, 2> a2_0{}; (void)a2_0;
- std::array<T, 2> a2_1{T()}; (void)a2_1;
- std::array<T, 2> a2_2{T(), T()}; (void)a2_2;
- }
- {
- std::array<T, 3> a3_0{}; (void)a3_0;
- std::array<T, 3> a3_1{T()}; (void)a3_1;
- std::array<T, 3> a3_2{T(), T()}; (void)a3_2;
- std::array<T, 3> a3_3{T(), T(), T()}; (void)a3_3;
- }
+ // A const empty array can also be default-initialized regardless of the type
+ // it contains. For non-empty arrays, this doesn't work whenever T doesn't
+ // have a user-provided default constructor.
+ {
+ const std::array<T, 0> a0;
+ (void)a0;
+ const std::array<NoDefault, 0> nodefault;
+ (void)nodefault;
+ }
- std::array<NoDefault, 0> nodefault{}; (void)nodefault;
- }
+ // Check direct-list-initialization syntax (introduced in C++11)
+#if TEST_STD_VER >= 11
+ {
+ {
+ std::array<T, 0> a0_0{};
+ (void)a0_0;
+ }
+ {
+ std::array<T, 1> a1_0{};
+ (void)a1_0;
+ std::array<T, 1> a1_1{T()};
+ (void)a1_1;
+ }
+ {
+ std::array<T, 2> a2_0{};
+ (void)a2_0;
+ std::array<T, 2> a2_1{T()};
+ (void)a2_1;
+ std::array<T, 2> a2_2{T(), T()};
+ (void)a2_2;
+ }
+ {
+ std::array<T, 3> a3_0{};
+ (void)a3_0;
+ std::array<T, 3> a3_1{T()};
+ (void)a3_1;
+ std::array<T, 3> a3_2{T(), T()};
+ (void)a3_2;
+ std::array<T, 3> a3_3{T(), T(), T()};
+ (void)a3_3;
+ }
+
+ std::array<NoDefault, 0> nodefault{};
+ (void)nodefault;
+ }
#endif
// Check copy-list-initialization syntax
@@ -119,7 +136,7 @@ struct test_initialization {
// See http://wg21.link/LWG2157
std::array<NoDefault, 0> nodefault = {{}}; (void)nodefault;
}
- }
+ }
};
// Test construction from an initializer-list
@@ -187,21 +204,21 @@ TEST_CONSTEXPR_CXX14 bool with_all_types()
#if TEST_STD_VER >= 20
template <class T>
concept is_list_initializable_int = requires {
- { T{123} };
+ { T{123} };
};
-struct Foo { };
+struct Foo {};
static_assert(!is_list_initializable_int<std::array<Foo, 0>>);
static_assert(!is_list_initializable_int<std::array<Foo, 1>>);
#endif
int main(int, char**)
{
- with_all_types<test_initialization>();
- test_initializer_list();
+ with_all_types<test_initialization>();
+ test_initializer_list();
#if TEST_STD_VER >= 14
- static_assert(with_all_types<test_initialization>(), "");
- static_assert(test_initializer_list(), "");
+ static_assert(with_all_types<test_initialization>(), "");
+ static_assert(test_initializer_list(), "");
#endif
return 0;
|
libcxx/include/array
Outdated
| @@ -280,7 +280,8 @@ struct _LIBCPP_TEMPLATE_VIS array<_Tp, 0> | |||
| typedef std::reverse_iterator<iterator> reverse_iterator; | |||
| typedef std::reverse_iterator<const_iterator> const_reverse_iterator; | |||
|
|
|||
| typedef __conditional_t<is_const<_Tp>::value, const char, char> _CharType; | |||
| struct _EmptyAggregate { }; | |||
| typedef __conditional_t<is_const<_Tp>::value, const _EmptyAggregate, _EmptyAggregate> _CharType; | |||
There was a problem hiding this comment.
| typedef __conditional_t<is_const<_Tp>::value, const _EmptyAggregate, _EmptyAggregate> _CharType; | |
| using _EmptyType = __maybe_const<_Tp>::value, _EmptyAggregate>; |
// #include <__type_traits/maybe_const.h> is needed too
The name was not great before, but now it looks really weird since there is no connection with char at all.
There was a problem hiding this comment.
I agree with renaming to _EmptyType, that was an oversight. However, I'd like to keep the __maybe_const out of this change, since it's really unrelated.
libcxx/test/std/containers/sequences/array/size_and_alignment.compile.pass.cpp
Outdated
Show resolved
Hide resolved
…h cases with const arrays.
libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp
Show resolved
Hide resolved
| static_assert(sizeof(Array) == sizeof(T), ""); | ||
| static_assert(TEST_ALIGNOF(Array) == TEST_ALIGNOF(T), ""); | ||
| static_assert(sizeof(Array) == sizeof(T[1]), ""); | ||
| static_assert(sizeof(Array) == sizeof(MyArray<T, 1>), ""); | ||
| static_assert(TEST_ALIGNOF(Array) == TEST_ALIGNOF(MyArray<T, 1>), ""); | ||
| static_assert(!std::is_empty<Array>::value, ""); |
There was a problem hiding this comment.
These look to me like they're all implementation-dependent. Am I missing some requirement, or should we make them libc++-specific?
There was a problem hiding this comment.
cppreference says:
This container is an aggregate type with the same semantics as a struct holding a C-style array
T[N]as its only non-static data member.
I think it's reasonable (and helpful for other implementations) to have these checks enabled for all implementations.
The tests for the 0-sized specialization could potentially be made libc++ only, but then again I think this will help enforce that all implementations provide compatible implementations.
There was a problem hiding this comment.
I was only talking about the zero-sized ones. The reality is that implementations already disagree on most things tested here: https://godbolt.org/z/75hqP4Yx6
There was a problem hiding this comment.
I see, I thought implementations agreed on this a bit more than they actually do.
|
Hmm, So I see we already migrated away from not returning 2 The "unique" bit specifically saying that the two arrays can't both return nullptr. I think we may have strayed from the right implementation here, and done it a while ago. I would like to consider this more. |
| // Before C++20, default initialization doesn't work inside constexpr for | ||
| // trivially default constructible types. This only apply to non-empty arrays, | ||
| // since empty arrays don't hold an element of type T. | ||
| if (TEST_STD_AT_LEAST_20_OR_RUNTIME_EVALUATED || !std::is_trivially_default_constructible<T>::value) { |
There was a problem hiding this comment.
I really hate logic in tests. The point where we have this many conditionals is the point where we should probably make the tests less generic and just spam the test out multiple times.
There was a problem hiding this comment.
I know -- I started like that and it was unreadable since I could hardly reuse anything at all, and I'm spamming exactly the same test over and over again for a bunch of different types. I dislike logic in tests, but I think this strikes the right balance after spending a bunch of time investigating different approaches.
| // This one isn't constexpr because omitting to initialize fundamental types | ||
| // isn't valid in a constexpr context. | ||
| struct test_default_initialization { | ||
| struct test_initialization { |
There was a problem hiding this comment.
Please make these separate tests rather than trying to reuse the instantiation. It's just clearer for readers.
There was a problem hiding this comment.
The only reason this was multiple structs previously is basically that I couldn't run the default initialization test in constexpr, so I needed another named struct for it. The way this is written right now follows our usual pattern of having something like:
constexpr bool test() {
// test case 1
{
...
}
// test case 2
{
...
}
// test case 3
{
...
}
return true;
}
int main() {
test();
static_assert(test());
}
| // template <class T, size_t N> | ||
| // struct array | ||
|
|
||
| // Make sure std::array<T, N> has the correct object size and alignment. |
There was a problem hiding this comment.
Only partially related but...
Any chance we've changed the triviality of any of the special members with this change?
There was a problem hiding this comment.
That's a great observation! This is tested in https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/containers/sequences/array/triviality.pass.cpp and I think this already has the appropriate coverage, but I am happy to add to it if you think we're missing something there.
Just to be clear though, this patch doesn't affect whether we return Edit: It looks like we raced on this! |
I may have found a possible way to do a standard compliant implementation, so don't merge this yet, I'll try to have a patch within the week. I'm not sure if it'll work fully yet and pass all tests but I found a hack with unions that might make the non-nullptr version implementable. |
|
Update: My fix doesn't work because of a different clang bug, so go ahead and merge this if it works, I think I can separately merge the other one if the other bug is fixed. |
|
I will merge this since it passes CI and looks like folks are satisfied. @EricWF if you want to talk about how I wrote the tests, I'm happy to go over it and change them in a future patch once we agree. |
This patch fixes constexpr default initialization of empty arrays and improves the tests accordingly.
Fixes #74375