This is an archive of the discontinued LLVM Phabricator instance.

[libc++][P0980] Marked member functions move/copy/assign of char_traits constexpr.
ClosedPublic

Authored by mpark on Oct 10 2019, 3:32 PM.

Event Timeline

mpark created this revision.Oct 10 2019, 3:32 PM
ldionne requested changes to this revision.Oct 11 2019, 9:41 AM
ldionne added inline comments.
libcxx/include/__string
227

I think it'd be better to have if (__libcpp_is_constant_evaluated()) { ... } else { ... } than just the early return. I normally don't care about this stuff, however in the case of is_constant_evaluated, I feel like having a clear if-else makes it clearer that we're doing the same thing in both branches, one in the compile-time world and one in the runtime world.

Also, it might be a good idea to define a __memmove_constexpr function that we can call as-is from the else branch?

This revision now requires changes to proceed.Oct 11 2019, 9:41 AM
mpark updated this revision to Diff 224682.Oct 11 2019, 2:33 PM

Added helper functions __move_constexpr, __copy_constexpr, and __assign_constexpr.

mpark marked an inline comment as done.Oct 11 2019, 2:35 PM
ldionne requested changes to this revision.Oct 16 2019, 6:51 AM

This is looking good. I especially like how the tests require only minimal change between the runtime and the compile-time version. I think this is what we should strive for pretty much every time we constexpr-ify something going forward.

libcxx/include/__string
199

This would be clearer (or something along those lines):

// constexpr version of the C functions of the same name
202

__memmove_constexpr?

215

__memcpy_constexpr?

223

__memset_constexpr?

664

Is that necessary?

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/assign3.pass.cpp
20

Did you forget to include the change to test_macros.h (or whatever file) where you add TEST_CONSTEXPR_CXX20? I can't find it.

Also, I would suggest TEST_CONSTEXPR_AFTER_CXX17, to mirror the checks we usually make (which are always of the form #if STD > 17.

This revision now requires changes to proceed.Oct 16 2019, 6:51 AM
ldionne marked an inline comment as done.Oct 16 2019, 6:53 AM
ldionne added inline comments.
libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/assign3.pass.cpp
20

Did you forget to include the change to test_macros.h (or whatever file) where you add TEST_CONSTEXPR_CXX20? I can't find it.

Nevermind this, I just saw the other review. My bad.

Also, I would suggest TEST_CONSTEXPR_AFTER_CXX17, to mirror the checks we usually make (which are always of the form #if STD > 17.

Forget this, I'll comment on D68837 instead.

mpark marked 2 inline comments as done.Oct 16 2019, 3:02 PM
mpark added inline comments.
libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/assign3.pass.cpp
20

Perhaps you decided to drop this issue since I don't see a follow-up comment on D68837.
Just as a note, I went with TEST_CONSTEXPR_CXX20 to be consistent with TEST_CONSTEXPR_CXX14.

mpark updated this revision to Diff 225315.Oct 16 2019, 3:10 PM
mpark marked 2 inline comments as done.

Elaborated on the constexpr comment.

libcxx/include/__string
202

I decided against this name here because this is more refactoring of the move algorithm for constexpr.
Specifically, we use this as the constexpr version of wmemmove as well as memmmove.

Same line of reasoning for the below requests.

664

Yes, this is necessary due to this test:

assert(std::char_traits<char16_t>::move(NULL, s1, 0) == NULL);

where, if n == 0, we should not perform the __s1 < __s2 check since that is UB.

mpark updated this revision to Diff 225498.Oct 17 2019, 12:39 PM

Minor formatting fixes.

ldionne accepted this revision.Nov 4 2019, 5:53 AM
This revision is now accepted and ready to land.Nov 4 2019, 5:53 AM
mpark updated this revision to Diff 228049.Nov 6 2019, 5:52 AM

Rebased on master.

mpark updated this revision to Diff 228256.Nov 7 2019, 8:50 AM

Rebased on top of the new is_constant_evaluated aware macro.

mpark updated this revision to Diff 228451.Nov 8 2019, 7:18 AM

Rebased

mpark updated this revision to Diff 228562.Nov 9 2019, 1:31 AM

Updated char_traits synopsis.

mpark updated this revision to Diff 228564.Nov 9 2019, 1:40 AM

Rebased on master.

This revision was automatically updated to reflect the committed changes.