This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Prepare string tests for constexpr
ClosedPublic

Authored by philnik on Mar 3 2022, 5:18 PM.

Details

Summary

These are the last™ changes to the tests for constexpr preparation.

Diff Detail

Event Timeline

philnik created this revision.Mar 3 2022, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 5:18 PM
philnik requested review of this revision.Mar 3 2022, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 5:18 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 413226.Mar 5 2022, 9:58 AM
  • Fix CI

LGTM%comments, and all the comments are pretty easy to deal with, I think.

libcxx/test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
24

Prefer TEST_CONSTEXPR (unless I'm missing a reason this doesn't work in C++11).

27

This either calls std::malloc or throws. I don't think it's safely constexpr-qualifiable even in C++20. Does it need to be?
Ditto deallocate.

41–67

Pre-existing: It'd be awesome to s/poca/pocca/ and move the operators into hidden friends. But in a separate PR for sure.

You put the poca_alloc codepath on lines 109-127 under !TEST_IS_CONSTANT_EVALUATED; is it possible that these constexpr-qualifications are unnecessary?

libcxx/test/std/strings/basic.string/string.cons/substr.pass.cpp
233–235

This is missing an >= 11 guard; but in fact I think what you should do here is remove this guard, and move the guards around test[_lwg]2583 inside the function body. Also some drive-by cleanup:

void test_lwg2583()
{
#if TEST_STD_VER >= 11 && defined(TEST_HAS_NO_EXCEPTIONS)
    typedef std::basic_string<char, std::char_traits<char>, test_allocator<char>> StringA;
    std::vector<StringA, std::scoped_allocator_adaptor<test_allocator<StringA>>> vs;
    StringA s{"1234"};
    vs.emplace_back(s, 2);
    try {
        vs.emplace_back(s, 5);
        assert(false);
    } catch (const std::out_of_range&) { }
#endif
}
libcxx/test/std/strings/basic.string/string.modifiers/string_append/iterator.pass.cpp
218–233

Here, I assume the only problem is with first's reinterpret_cast; the other 3 calls to test are OK, right? Consider refactoring to pull out first into its own code block (at the expense of some duplication, I assume, but that's fine).

philnik updated this revision to Diff 413927.Mar 8 2022, 1:30 PM
philnik marked 5 inline comments as done.
  • Address comments
libcxx/test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
41–67

Yep, it only has to be a constexpr type, so making the constructors constexpr is enough.

EricWF added a subscriber: EricWF.Mar 8 2022, 5:36 PM

Doesn't this change make all of the tests with functions decorated with TEST_CONSTEXPR_AFTER_CXX20 undefined behavior, since it's undefined to have constexpr on a function that is never constexpr?

libcxx/test/std/strings/basic.string/string.contains/contains.string_view.pass.cpp
93

Please don't go adding commented out tests in the future.

libcxx/test/std/strings/basic.string/string.modifiers/string_assign/iterator.pass.cpp
34

Is this ever constexpr?

philnik marked 2 inline comments as done.Mar 10 2022, 11:20 AM
philnik added inline comments.
libcxx/test/std/strings/basic.string/string.contains/contains.string_view.pass.cpp
93

This was requested by Louis.

philnik updated this revision to Diff 414443.Mar 10 2022, 11:20 AM
philnik marked an inline comment as done.
  • Address comments

@EricWF How could this be undefined bahavior? All the tests are templated on the string, so it's possible to give them a constexpr string.

Doesn't this change make all of the tests with functions decorated with TEST_CONSTEXPR_AFTER_CXX20 undefined behavior, since it's undefined to have constexpr on a function that is never constexpr?

@EricWF How could this be undefined bahavior? All the tests are templated on the string, so it's possible to give them a constexpr string.

This is theory versus practice. In theory, it's IFNDR to define a template that can never be validly instantiated. But in practice the meaning of "can never" is really unclear: does it mean "is never"? how much imagination does the compiler have to possess here? So in practice, compilers don't check template definitions prior to instantiation, and thus templates are always safe to mark constexpr. We're using that practical license here. I don't believe there's any practical problem with what we're doing here, IFNDR-wise. (And within a couple of months even the theoretical question should be moot anyway, right? hopefully?)

Mordante accepted this revision.Mar 19 2022, 10:33 AM
Mordante added a subscriber: Mordante.

LGTM! Just curious did you test locally whether these are really the final preparation changes required?
Note I don't see that as blocking, the additional changes can be done when std::string is made constexpr.

This revision is now accepted and ready to land.Mar 19 2022, 10:33 AM

LGTM! Just curious did you test locally whether these are really the final preparation changes required?
Note I don't see that as blocking, the additional changes can be done when std::string is made constexpr.

I have a (almost) working constexpr std::string version locally and these should be the last diffs in the test files other than actually enabling the tests. I had to cherry-pick these changes manually, so I might have missed something.

This revision was automatically updated to reflect the committed changes.