Page MenuHomePhabricator

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

Authored by philnik on Oct 2 2019, 5:42 PM.

Details

Reviewers
mclow.lists
EricWF
rsmith
ldionne
Group Reviewers
Restricted Project

Diff Detail

Unit TestsFailed

TimeTest
4,040 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp
Script: -- : 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
4,040 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp
Script: -- : 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
4,040 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp
Script: -- : 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
4,040 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp
Script: -- : 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
4,040 mslibcxx CI C++11 > llvm-libc++-shared-cfg-in.libcxx::clang_tidy.sh.cpp
Script: -- : 'RUN: at line 12'; clang-tidy /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp --warnings-as-errors=* -header-filter=.* -- -Wweak-vtables -Wno-unknown-warning-option -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/build/generic-cxx11/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/0be5cb96ef29-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++11 -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fno-modules
View Full Test Results (21,280 Failed)

Event Timeline

rsmith created this revision.Oct 2 2019, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 5:42 PM
Herald added a subscriber: christof. · View Herald Transcript
rsmith updated this revision to Diff 222945.Oct 2 2019, 5:48 PM

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

ldionne added a subscriber: ldionne.Oct 3 2019, 7:24 AM

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

ldionne commandeered this revision.Oct 3 2019, 7:44 AM
ldionne added a reviewer: rsmith.
ldionne updated this revision to Diff 302609.Nov 3 2020, 10:13 AM

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.

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 3 2020, 10:13 AM
miscco added a subscriber: miscco.Nov 3 2020, 11:14 AM

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 updated this revision to Diff 427645.May 6 2022, 8:20 AM
ldionne added a subscriber: philnik.

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.

Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 8:20 AM
Herald added a subscriber: ormris. · View Herald Transcript
philnik commandeered this revision.May 6 2022, 8:23 AM
philnik added a reviewer: ldionne.
philnik updated this revision to Diff 427760.May 6 2022, 2:51 PM
philnik marked 5 inline comments as done.
  • 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 retitled this revision from Prototype implementation of P1004R2 (making std::vector constexpr). to Implement P1004R2 (constexpr std::vector).
philnik edited the summary of this revision. (Show Details)
philnik updated this revision to Diff 427832.May 7 2022, 2:20 AM
  • Try to fix CI
philnik updated this revision to Diff 427839.May 7 2022, 3:21 AM
  • Next try
philnik updated this revision to Diff 427845.May 7 2022, 4:31 AM
  • Try to fix GCC
philnik retitled this revision from Implement P1004R2 (constexpr std::vector) to [libc++] Implement P1004R2 (constexpr std::vector).May 7 2022, 4:43 AM
philnik updated this revision to Diff 427927.May 8 2022, 5:26 AM
  • Try to fix CI
philnik updated this revision to Diff 431097.May 20 2022, 4:57 PM
  • Rebased
  • Fix parts of CI
philnik updated this revision to Diff 436255.Sun, Jun 12, 2:53 PM
  • Try to fix C++03
ldionne requested changes to this revision.Thu, Jun 16, 12:01 PM
ldionne added a subscriber: var-const.
ldionne added inline comments.
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?

This revision now requires changes to proceed.Thu, Jun 16, 12:01 PM
philnik updated this revision to Diff 440151.Mon, Jun 27, 3:18 AM
philnik marked 10 inline comments as done.
  • Address comments
philnik added inline comments.Mon, Jun 27, 3:18 AM
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 added inline comments.Sat, Jul 2, 2:36 PM
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.