This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Enable constexpr string comparison tests
ClosedPublic

Authored by jloser on May 31 2022, 5:26 PM.

Details

Summary

Some tests in string.view.comparison are not enabled due to previous lack of
support for constexpr std::string. Now that it is implemented, we can enable
these tests.

Diff Detail

Event Timeline

jloser created this revision.May 31 2022, 5:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 5:26 PM
jloser requested review of this revision.May 31 2022, 5:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2022, 5:26 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser planned changes to this revision.May 31 2022, 9:27 PM

@philnik I'm not sure there's actually anything to do here - do you see anything? We do need the TEST_IS_CONSTANT_EVALUATED check so the test won't run in C++14 or C++17 modes for example. So, I think top of tree is fine as-is, unless I'm missing something.

philnik added inline comments.Jun 1 2022, 12:57 AM
libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp
10

Pre-existing: These tests should only be enabled for libc++ pre-C++17.

55–56

These tests should be enabled for C++20 constant evaluation. You probably have to add a function or macro like is_cxx20_or_runtime_evaluated().

jloser added inline comments.Jun 1 2022, 7:23 AM
libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp
10

I don't think so, since we backport string_view all the way back to C++03 as an extension.

jloser updated this revision to Diff 433391.Jun 1 2022, 7:30 AM

Enable the tests for C++20

philnik accepted this revision.Jun 1 2022, 9:24 AM

LGTM assuming CI passes. It would be nice if you could disable the tests for other standard libraries pre-C++17.

libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp
10

Sorry, my comment was a bit unclear: It should only be enabled for libc++ pre-C++17. i.e. not for libstdc++, MSVC STL etc. I'm not 100% sure, but I think // UNSUPPORTED: !stdlib=libc++ && (c++03 || c++11 || c++14) should work.

This revision is now accepted and ready to land.Jun 1 2022, 9:24 AM
jloser added inline comments.Jun 1 2022, 9:59 AM
libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp
10

I see. Yeah, I agree with that. That's a pre-existing thing though and I'd recommend we do that for all of the string_view tests. I'm happy to volunteer to do that as a follow-up if that works for you.

The ranges::transform test timeouts are normal. You can consider the CI passed.

libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp
10

Ok yeah. If they are missing for all tests it should probably be done in a separate PR.

Mordante accepted this revision.Jun 1 2022, 11:58 AM

Nice catch! LGTM! It seems the build errors are time-out and they don't seem to be related to your changes.

This revision was landed with ongoing or failed builds.Jun 1 2022, 6:28 PM
This revision was automatically updated to reflect the committed changes.