This is an archive of the discontinued LLVM Phabricator instance.

[libc++][P0784] Marked the default allocator constexpr.
AbandonedPublic

Authored by mpark on Oct 17 2019, 12:45 PM.

Details

Reviewers
ldionne
EricWF
mclow.lists
Group Reviewers
Restricted Project

Event Timeline

mpark created this revision.Oct 17 2019, 12:45 PM
mpark updated this revision to Diff 225502.Oct 17 2019, 12:50 PM

Fixed allocate.size.fail.cpp test.

ldionne requested changes to this revision.Nov 4 2019, 6:28 AM
ldionne added inline comments.
libcxx/include/memory
1873–1874

Isn't it fine to throw inside constexpr if we can't allocate?

1999–2000

Ditto

This revision now requires changes to proceed.Nov 4 2019, 6:28 AM

Woops, it also looks like you are missing the feature test macros:

Add to Table 17: Feature-test macros with the appropriate value:
__cpp_constexpr_dynamic_alloc
Add to Table 36: Standard library feature-test macros with the appropriate value:
__cpp_lib_constexpr_dynamic_alloc <memory>

mpark updated this revision to Diff 228020.Nov 6 2019, 1:21 AM

Removed unnecessary calls to is_constant_evaluated.

mpark marked 2 inline comments as done.Nov 6 2019, 1:22 AM
mpark added a comment.Nov 6 2019, 1:29 AM

I'm planning to add __cpp_lib_constexpr_dynamic_alloc in a later patch, after the rest of P0784.

mpark updated this revision to Diff 228271.Nov 7 2019, 9:56 AM

Updated to check for __cpp_constexpr_dynamic_alloc.

mpark updated this revision to Diff 228452.Nov 8 2019, 7:20 AM

Rebased

mpark updated this revision to Diff 228857.Nov 12 2019, 4:16 AM

Rebased on top of D70117.

mpark updated this revision to Diff 228976.EditedNov 12 2019, 3:35 PM

Marked max_size, construct and destroy constexpr as well.

mpark retitled this revision from [libc++][P0784] Marked allocator constexpr. to [libc++][P0784] Marked default allocator constexpr..Nov 12 2019, 3:35 PM
mpark retitled this revision from [libc++][P0784] Marked default allocator constexpr. to [libc++][P0784] Marked the default allocator constexpr..
mpark updated this revision to Diff 229008.Nov 12 2019, 10:30 PM

Rebased.

ldionne requested changes to this revision.Nov 13 2019, 8:30 AM
ldionne added inline comments.
libcxx/include/memory
1957

This is because __cpp_constexpr_dynamic_alloc also controls whether you can call a destructor explicitly inside constexpr, right? It looks kind of funny, because we don't allocate here.

EDIT: I guess the same hold for construct, where we use placement-new.

libcxx/include/new
261

I assume you had to move the __do_call functions here because making them constexpr somehow required their definition to appear above the definition of the various __do_allocate functions below?

libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.fail.cpp
29

I don't understand that test. Shouldn't allocator be made constexpr by this patch? Why do you check that there's an error saying it's not an ICE?

libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp
83

You have a mismatch in the indentation of the return and body.

libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.size.fail.cpp
38

I would expect that you get one error for each statement up in test() above. Isn't it what's happening? I would expect something like expected-error 4 {{...}}.

39

Why not just static_assert here?

This revision now requires changes to proceed.Nov 13 2019, 8:30 AM
mpark marked 4 inline comments as done and an inline comment as not done.Feb 14 2020, 8:34 AM
mpark added inline comments.
libcxx/include/new
261

That's correct.

libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp
83

Yeah, I was following the formatting of many of tests. See libcxx/test/std/utilities/memory/default.allocator/allocator_types.pass.cpp for an example, but there are many others.

mpark marked 5 inline comments as done.Feb 14 2020, 8:35 AM
mpark added inline comments.
libcxx/include/memory
1957

Yep, that's also correct.

mpark marked an inline comment as done.Feb 14 2020, 8:44 AM

This is superseded by https://reviews.llvm.org/D68364. Thanks a lot for your work on this @mpark, I based my patch on top of yours. I tried taking all the comments from this review and incorporating them into D68364.

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 17 2020, 1:15 PM
mpark abandoned this revision.Sep 17 2020, 1:20 PM