This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [test] Refactor string_view comparison tests for comprehensiveness.
ClosedPublic

Authored by Quuxplusone on Nov 26 2021, 3:25 PM.

Details

Summary

There are two commits here:

[libc++] [test] C++03-friendly MAKE_STRING macro.

Used in the new string_view test in this PR. (We support string_view in C++03. I don't object if we want to just stop doing that.)

[libc++] [test] Refactor string_view comparison tests for comprehensiveness.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Nov 26 2021, 3:25 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptNov 26 2021, 3:25 PM

Fix TEST_HAS_NO_WIDE_CHARACTERS build, and remove some trailing whitespace.

libcxx/test/std/strings/string.view/string.view.comparison/opeq.pass.cpp
49–52 ↗(On Diff #390125)

I expected these to fail-to-compile on clang-cl, for the reasons discussed (at length!) in D113161; but it didn't fail, at least not on buildkite. Strange.

jloser added inline comments.Nov 26 2021, 5:58 PM
libcxx/test/std/strings/string.view/string.view.comparison/opeq.pass.cpp
49–52 ↗(On Diff #390125)

Any ideas why it doesn't fail on clang-cl on buildkit?

libcxx/test/std/strings/string.view/string.view.comparison/opeq.pass.cpp
49–52 ↗(On Diff #390125)

Since these functions are small inline functions, they probably just get inlined everywhere, so the colliding symbols are never actually generated. I hypothesize that clang-cl might not bother to complain unless the symbols are actually generated (e.g. by taking the addresses of these functions, or by failing to inline them). Of course then I still don't understand why it was complaining in your case and not mine.

Nice cleanup. I see quite a bit of repetition in the tests, but I don't see a simple way to avoid that.

libcxx/test/support/test_macros.h
154

As you already mentioned in your commit message these are unused and don't look too pretty. I would prefer to not add them until we really need them.

Quuxplusone retitled this revision from [WIP] [libc++] [test] Several test-code refactorings. to [libc++] [test] Refactor string_view comparison tests for comprehensiveness..
Quuxplusone edited the summary of this revision. (Show Details)

Abandon the TEST_IS_CONSTANT_EVALUATED -> TEST_IS_RUNTIME refactoring, until it becomes needed.
Rebased. I'm calling this "ready for review."

Any idea what the existing motivation is to support string_view all the way back to C++03?

ldionne requested changes to this revision.Nov 29 2021, 8:24 AM

Any idea what the existing motivation is to support string_view all the way back to C++03?

It was seen as a really useful type and at the time it was thought that it would be beneficial for users that are stuck in C++03 to be able to use this. After being bit by backporting features a couple of times, we don't do this anymore and we instead encourage people to upgrade to a newer version of the standard.

libcxx/test/std/strings/string.view/string.view.comparison/opge.pass.cpp
1 ↗(On Diff #390212)

Can you name the new tests something less terse, e.g. equal.pass.cpp and so on?

libcxx/test/std/strings/string.view/string.view.comparison/opgt.pass.cpp
12 ↗(On Diff #390212)

Nitpick. Applies elsewhere too.

libcxx/test/std/strings/string.view/string.view.comparison/opne.pass.cpp
48–52 ↗(On Diff #390212)

Can you link to http://eel.is/c++draft/string.view#tab:string.view.comparison.overloads so it's easy to see what you are testing for exhaustiveness? Same for other operators.

This revision now requires changes to proceed.Nov 29 2021, 8:24 AM
Quuxplusone marked 3 inline comments as done.

Fix TEST_IS_CONSTEXPR for C++14/17.
Rename the new test files per @ldionne.

Mordante accepted this revision as: Mordante.Nov 30 2021, 9:03 AM

LGTM!

ldionne accepted this revision.Nov 30 2021, 1:17 PM

Thanks!

libcxx/test/std/strings/string.view/string.view.comparison/less_equal.pass.cpp
45

Here and in other places -- this is unusual.

This revision is now accepted and ready to land.Nov 30 2021, 1:17 PM
libcxx/test/std/strings/string.view/string.view.comparison/opne.string_view.string.pass.cpp