[libc++] Implement P1004R2 (constexpr std::vector)

[rsmith]

Move some changes from here to the std::allocator change.

[ldionne]

I'll pick this up. I wasn't aware you had started working on this, but thanks!

[ldionne]

I've had a revision locally for a long long time. It doesn't pass everything
yet, but I've managed to get it down to ~10 failing tests with a few more hacks
locally.

[miscco]

Some quick review of the the acutal changes to the headers

libcxx/include/memory

884–885

I am wondering whether we should reuse the simplementation of std::copy_n here (or at the call site for what its worth)

884–885

As said above, we should be able to reuse the internals of std::copy_n here

884–885

I believe the inline is superfluous with the _LIBCPP_CONSTEXPR_AFTER_CXX17

Appears throughout

884–885

From here and below there is _LIBCPP_CONSTEXPR_AFTER_CXX11 rather than _LIBCPP_CONSTEXPR_AFTER_CXX17

Also the ordering of _LIBCPP_INLINE_VISIBILITY and _LIBCPP_CONSTEXPR_AFTER_CXX11 is swapped.

libcxx/include/vector

332–333

Many inline below here

[ldionne]

Rebase onto main.

@miscco's comments have not been addressed yet -- I only did the minimal amount of work to fix merge conflicts with main.

Note that not all tests have been constexpr-ified yet, and the ones that have been constexpr-ified are not all passing. Transferring to @philnik as discussed offline.

[philnik]

• Fix tests

libcxx/include/memory

884–885

I think this and the comment below have been addressed. But I'm not 100% sure what you were referring to exactly. Could you comment again at the correct line if the comments haven't been addressed?

884–885

Sorry, I don't know what you are referring to again. I guess they have been addressed since I can't see any _LIBCPP_CONSTEXPR_AFTER_CXX11 of inline in the diff.

libcxx/include/vector

332–333

Again, I guess it has been addressed?

[philnik]

• Try to fix CI

[philnik]

• Next try

[philnik]

• Try to fix GCC

[philnik]

• Try to fix CI

[philnik]

• Rebased
• Fix parts of CI

[philnik]

• Try to fix C++03

[ldionne]

libcxx/include/__bit_reference

358–359

I think we should always call std::fill_n here instead. Indeed, both are equivalent, but std::fill_n is nicer for various reasons.

I know fill_n doesn't end up calling memset directly, however the compiler will end up calling memset (when optimized). IIRC, @var-const was actually about to add this optimization to fill_n explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm?

Also, I think we need to add a comment in std::fill_n that explains this. I think it can be done in this patch since we're switching from memset to fill_n.

This comment applies elsewhere in this patch too.

497

Why is this required?

957–960

Instead, I would begin the lifetime of the elements inside __bit_array's constructor if we are during constant evaluation. That way you shouldn't need to change this code at all.

libcxx/include/__memory/construct_at.h

40–41

Please match the style of the class = decltype above, just for consistency.

46

Let's add the _LIBCPP_ASSERT here too.

libcxx/include/__utility/move.h

40–49 ↗

(On Diff #436255)

I don't think this diff is needed in this patch, see my comment below.

libcxx/include/memory

884–885

Why are we using std::__copy here? Let's use std::copy instead.

884–885

Given that __begin2 = std::__copy(__begin1, __end1, __begin2); does not compile, I think this overload is never used. Can you please confirm this by adding static_assert(sizeof(_Alloc) == 0, "") and running the tests? If the tests don't fail, then this is a dead code path and we'll need to investigate it.

Edit Well, it looks like we've got enable_if<cond>::type above as a template parameter. IOW, we're declaring a NTTP of type void, and that's never valid. 🤦🏼‍♂️ Clang should really tell us about that. Let's fix it in a separate patch prior to this one.

884–885

I think this should all just be uninitialized_copy (or a __ version of it).

885

I don't think we *need* to make this change to enable constexpr vector, right? If not, let's pull it into a separate patch, cause I'd like to have a discussion about it. Otherwise, can you please explain why it's needed?

[philnik]

• Address comments

[philnik]

libcxx/include/__bit_reference

497

It's not. I probably changed that during some debugging.

libcxx/include/memory

884–885

Just for reference, this is D128146 now.

[var-const]

libcxx/include/__bit_reference

358–359

I can confirm that working on https://reviews.llvm.org/D118329, I found that the compiler would replace uninitialized_fill{,_n} with calls to memset in an optimized build, which made it unnecessary to write that optimization explicitly. I would expect fill_n to work similarly, but I have only tried with the uninitialized algorithms. FWIW, I have a lingering suspicion that there might be some corner cases (related to aliasing, perhaps) where the compiler would fail to do the optimization, but it was absolutely reliable in all the (simple) cases I checked.