Specially handle `array<T, 0>` for non-default-constructible `T` by CaseyCarter · Pull Request #2296 · microsoft/STL

Casey Carter (CaseyCarter) · 2021-10-22T19:16:12Z

Previously, these types were non-instantiable; we can't break ABI for something that doesn't compile. (Why do I always end up coding the PRs that fire salvos at the ODR?)

Fixes #942
Fixes VSO-207715 / AB#207715.

Note that the proposed resolution of LWG-2157 clarifies how array<T, 0> should behave. It hasn't been merged yet due to many of us despising the bit about "When a and b are distinct objects of the same zero-sized array type, a.begin() and b.begin() are not iterators over the same underlying sequence. [Note: Therefore begin() does not return a value-initialized iterator — end note]". If begin and end cannot return a value-initialized iterator (either a nullptr or a wrapper around a nullptr) it's not clear how they are to be implemented so as to be usable in constexpr context.

After applying this change, we will (at least in some cases) agree with the behavior of libc++ and libstdc++ which both return iterator{data()} with data() being nullptr. I will use this as evidence that the implementations don't agree with the cited bit of LWG-2157 and hopefully convince LWG to yoink that and merge the rest.

Previously, these types were non-instantiable; we can't break ABI for something that doesn't compile. Fixes microsoft#942

Casey Carter (CaseyCarter) · 2021-10-22T19:49:58Z

    }

-    _Ty _Elems[1];
+    conditional_t<is_default_constructible_v<_Ty>, _Ty, char> _Elems[1];


This could just as well be a char as char[1] - the only requirement is that {} is a valid initializer. (If we were breaking ABI, I'd have array<T, 0> inherit from an empty base type so it could be empty and still initializable with {{}}. Such a beautiful hack!)

Anyone see a reason to prefer either char or char[1]?

I would prefer something with [[msvc:no_unique_address]], if clang had that.

And I prefer char[1], as it is, for no particular reason, just because it is an array

I would prefer something with [[msvc:no_unique_address]], if clang had that.

Good point! An empty aggregate member would work with [[no_unique_address]] in ABI-broken world.

And I prefer char[1], as it is, for no particular reason, just because it is an array

Yeah, I have vague concerns that code "somewhere" outside this header expects array::_Elems to exist and have array type.

Michael Schellenberger Costa (miscco)

I hate array<T, 0> even more than I fear input_iterator

Alex Guteniev (AlexGuteniev) · 2021-10-22T20:05:45Z

Why do I always end up coding the PRs that fire salvos at the ODR?

Because you're an STL maintainer, Casey.

timsong-cpp · 2021-10-23T06:41:43Z

Previously, these types were non-instantiable; we can't break ABI for something that doesn't compile. (Why do I always end up coding the PRs that fire salvos at the ODR?)

I don't think "non-instantiable" is correct.

#include <array>

struct A { const int i; };

static_assert(not std::is_default_constructible_v<A>);
// std::array<A, 0> a; // Error, but...
std::array<A, 0> a = {}; // OK

Also, it'd be nice if copying a default-initialized array of zero doesn't give me core language undefined behavior from copying an uninitialized char.

Casey Carter (CaseyCarter) · 2021-10-25T03:04:22Z

Previously, these types were non-instantiable; we can't break ABI for something that doesn't compile. (Why do I always end up coding the PRs that fire salvos at the ODR?)

I don't think "non-instantiable" is correct.

Thank you for the reminder that default-constructible doesn't mean the same thing as default-initializable! I've adjusted the new behavior so it's enabled only when the array element type cannot be copy-initialized with {}.

Also, it'd be nice if copying a default-initialized array of zero doesn't give me core language undefined behavior from copying an uninitialized char.

Fair enough.

timsong-cpp · 2021-10-25T03:26:34Z

    }

-    _Ty _Elems[1];
+    conditional_t<_Is_implicitly_default_constructible<_Ty>::value, _Ty, _Empty_array_element> _Elems[1];


I think this needs to check either? If is_default_constructible is true then array<T, 0> could be default-initialized (or value-initialized), while if _Is_implicitly_default_constructible is true then array<T, 0> could be copy-list-initialized from {}. Either way you have ABI issues.

I have a hard time believing that default-constructible types that are not {}-constructible are so common that someone out there has an array of them compiled into an object file, but since you went to the trouble to make the comment I'll make the change.

MikeGitb (MikeGitb) · 2021-10-25T07:54:33Z

After applying this change, we will (at least in some cases) agree with the behavior of libc++ and libstdc++ which both return iterator{data()} with data() being nullptr.

I guess it makes sense to follow what other implementations do, but wouldn't it be much more consistent if it data would point to the start of the array object itself, just like for any other std::array type? Asked differently: What is the motivation to return a nullptr there (assuming you know)?

Casey Carter (CaseyCarter) · 2021-10-25T21:21:50Z

After applying this change, we will (at least in some cases) agree with the behavior of libc++ and libstdc++ which both return iterator{data()} with data() being nullptr.

I guess it makes sense to follow what other implementations do, but wouldn't it be much more consistent if it data would point to the start of the array object itself, just like for any other std::array type? Asked differently: What is the motivation to return a nullptr there (assuming you know)?

Returning something other than nullptr from data at constexpr time requires that you have storage for an actual T in a union. Ideally array<T, 0> would be an empty, trivially-copyable type, but carrying a T in a union means it must have at least the size and alignment of a T, and means trivial-copyability can be at best conditional. Using nullptr simplifies the implementation, and makes it easier to catch bugs where the user calls front/back or otherwise dereferences begin() since a null pointer value is very obviously invalid.

MikeGitb (MikeGitb) · 2021-10-25T22:17:32Z

Returning something other than nullptr from data at constexpr time requires that you have storage for an actual T in a union.

Right, I forgot about constexpr and that T arr[0] isn't allowed. Thanks for the explanation.

and makes it easier to catch bugs where the user calls front/back

Shouldn't those trigger a compilation error?

Casey Carter (CaseyCarter) · 2021-10-26T19:41:29Z

and makes it easier to catch bugs where the user calls front/back

Shouldn't those trigger a compilation error?

WG21 tries to avoid API discontinuity for particular template specializations to make it easier to write generic code. For example, you might write if (!meow.empty()) { do_stuff_with(meow.front()); } in a function that generically accepts sequence containers. This fails to compile if meow is an array-of-zero and front is ill-formed, but is perfectly correct if front is well-formed but can't be called.

Stephan T. Lavavej (StephanTLavavej) · 2021-11-12T05:26:06Z

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed, or if more work is required.

Stephan T. Lavavej (StephanTLavavej) · 2021-11-13T08:31:11Z

Thanks for fixing this previously-thought-to-be-impossible bug! 🚀 🪐 😸

Specially handle array<T, 0> for non-default-constructible T

d50e1c0

Previously, these types were non-instantiable; we can't break ABI for something that doesn't compile. Fixes microsoft#942

Casey Carter (CaseyCarter) added the bug Something isn't working label Oct 22, 2021

Casey Carter (CaseyCarter) commented Oct 22, 2021

View reviewed changes

Casey Carter (CaseyCarter) marked this pull request as ready for review October 22, 2021 19:51

Casey Carter (CaseyCarter) requested a review from a team as a code owner October 22, 2021 19:51

Michael Schellenberger Costa (miscco) approved these changes Oct 22, 2021

View reviewed changes

Stephan T. Lavavej (StephanTLavavej) approved these changes Oct 23, 2021

View reviewed changes

Tim's review comments

a2607b2

timsong-cpp reviewed Oct 25, 2021

View reviewed changes

Tim's repeated review comment =)

69b708e

Stephan T. Lavavej (StephanTLavavej) approved these changes Oct 27, 2021

View reviewed changes

Stephan T. Lavavej (StephanTLavavej) assigned Charlie Barto (barcharcraz) Oct 28, 2021

Stephan T. Lavavej (StephanTLavavej) self-assigned this Nov 12, 2021

Stephan T. Lavavej (StephanTLavavej) merged commit 793713e into microsoft:main Nov 13, 2021

Casey Carter (CaseyCarter) deleted the array_zero branch November 14, 2021 19:50

Stephan T. Lavavej (StephanTLavavej) mentioned this pull request Jan 19, 2022

<future>: promise default constructs the value type #2488

Open

Marcel Jacobse (mjacobse) mentioned this pull request Dec 5, 2023

[libc++] Initialization of std::array of size 0 in constexpr contexts llvm/llvm-project#74375

Closed

Uh oh!

Conversation

Casey Carter (CaseyCarter) commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Casey Carter (CaseyCarter) Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alex Guteniev (AlexGuteniev) Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

Casey Carter (CaseyCarter) Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

Michael Schellenberger Costa (miscco) left a comment

Choose a reason for hiding this comment

Uh oh!

Alex Guteniev (AlexGuteniev) commented Oct 22, 2021

Uh oh!

timsong-cpp commented Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Casey Carter (CaseyCarter) commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timsong-cpp Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Casey Carter (CaseyCarter) Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

MikeGitb (MikeGitb) commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Casey Carter (CaseyCarter) commented Oct 25, 2021

Uh oh!

MikeGitb (MikeGitb) commented Oct 25, 2021

Uh oh!

Casey Carter (CaseyCarter) commented Oct 26, 2021

Uh oh!

Stephan T. Lavavej (StephanTLavavej) commented Nov 12, 2021

Uh oh!

Stephan T. Lavavej (StephanTLavavej) commented Nov 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Casey Carter (CaseyCarter) commented Oct 22, 2021 •

edited

Loading

Casey Carter (CaseyCarter) Oct 22, 2021 •

edited

Loading

timsong-cpp commented Oct 23, 2021 •

edited

Loading

Casey Carter (CaseyCarter) commented Oct 25, 2021 •

edited

Loading

MikeGitb (MikeGitb) commented Oct 25, 2021 •

edited

Loading