Details
- Reviewers
ldionne • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rGdcffa7d3e140: [libc++] Prepare string.modifiers tests for constexpr
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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? |
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. |
libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp | ||
---|---|---|
1832 |
Interesting -- things are never as simple as they seem. I'd still like for test31 to be a template, like the other ones. |
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. |
Instead, can you do
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.