This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Prepare string.modifiers tests for constexpr
ClosedPublic

Authored by philnik on Feb 9 2022, 6:16 AM.

Details

Diff Detail

Event Timeline

philnik requested review of this revision.Feb 9 2022, 6:16 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2022, 6:16 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Feb 9 2022, 8:22 AM

Looks pretty good, with some comments.

libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp
1832

Instead, can you do

template <class S, class V>
_LIBCPP_CONSTEXPR_AFTER_CXX17 void tests() {
  test0<S, SV>();
  // ...
  test31<S, SV>();
}

bool check_all() {
  tests<std::string, std::string_view>();
#if TEST_STD_VER >= 11
  tests<std::basic_string<char, std::char_traits<char>, min_allocator<char>>,
        std::basic_string_view<char, std::char_traits<char>>>();
#endif
  return true;
}

int main() {
  check_all();
#if TEST_STD_VER > 17
  static_assert(check_all());
#endif
}

You can also tweak test31 to be templated like the other ones, I think you only need to use the SV() macro and friends in it.

libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_string_size_size.pass.cpp
1861

Same comment as above.

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_iter_iter.pass.cpp
1047

Same comment as above.

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_pointer_size.pass.cpp
983

Same comment as above.

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_T_size_size.pass.cpp
6020

Same comment as above.

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_pointer_size.pass.cpp
1324

Here too.

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_string_size_size.pass.cpp
6094

Here too.

This revision now requires changes to proceed.Feb 9 2022, 8:22 AM
philnik added inline comments.Feb 9 2022, 8:46 AM
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp
1832

I can't, because you can "only" do about 1 million steps during constant evaluation. If everything is in a single static_assert() that limit is reached. I can of course make test 31 a template.

Quuxplusone added inline comments.Feb 9 2022, 8:58 AM
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp
1832

FWIW, a quick skim did not enlighten me as to why we need exactly these 32 opaquely numbered test cases here. What's the purpose of each of these tests? Can we get the "length" of this test down below a million simply by improving it?
I see we might want to test every possible cell in the matrix (short,long) x (0,1,3,n-1,n,n+1) x (short,long) x (0,1,3,n-1,n,n+1) x (0,1,2,3,4,n-1,n,n+1) but even that's only 1152 different test cases, and we've got 1843 lines in the left-hand file here.

philnik added inline comments.Feb 9 2022, 10:22 AM
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp
1832

I don't think we would get it below a million without decreasing test coverage. If I remember correctly clang compiled about the first 10 functions until it errored. Anyways, that's definitely out of scope for this PR.

philnik updated this revision to Diff 407278.Feb 9 2022, 1:34 PM
  • Fix CI
ldionne added inline comments.Feb 9 2022, 1:51 PM
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp
1832

I can't, because you can "only" do about 1 million steps during constant evaluation. If everything is in a single static_assert() that limit is reached. I can of course make test 31 a template.

Interesting -- things are never as simple as they seem.

I'd still like for test31 to be a template, like the other ones.

philnik updated this revision to Diff 407537.Feb 10 2022, 7:51 AM
philnik marked 5 inline comments as done.
  • Make testN templates
ldionne accepted this revision.Feb 10 2022, 8:00 AM
ldionne added inline comments.
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp
1832

Hmm, incidentally, I thought we were instantiating those tests with wchar_t, not only char. That's why I said "use the SV macro", confusing it with the MAKE_STRING macro we use elsewhere. My comment must not have made much sense. Anyway, LGTM.

This revision is now accepted and ready to land.Feb 10 2022, 8:00 AM
This revision was landed with ongoing or failed builds.Feb 10 2022, 12:43 PM
This revision was automatically updated to reflect the committed changes.