This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format[[nfc] Use string_view in tests.
ClosedPublic

Authored by Mordante on Feb 1 2022, 10:37 AM.

Details

Summary

This change is a preparation for adapting the tests for

P2216 std::format improvements

Diff Detail

Event Timeline

Mordante requested review of this revision.Feb 1 2022, 10:37 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 10:37 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/test/std/utilities/format/format.functions/format.locale.pass.cpp
40–42

FWIW, you could use const auto&... args to avoid such a long line (and to remove one C++20ism, although obviously removing the other template parameter would require more surgery).

libcxx/test/std/utilities/format/format.functions/format_to.pass.cpp
69

Pre-existing nit presumably throughout: "Catch by const reference" is a good habit.
https://quuxplusone.github.io/blog/2018/09/16/data-race-when-catch-by-nonconst-reference/

libcxx/test/std/utilities/format/format.functions/vformat_to.locale.pass.cpp
33

Why not std::basic_string_view<CharT> expected as well? (Ditto throughout.)

Mordante marked an inline comment as done.Feb 1 2022, 12:11 PM

Thanks for the review.

libcxx/test/std/utilities/format/format.functions/format.locale.pass.cpp
40–42

The test is only needed in C++20 ;-). I prefer to keep the current syntax. The const Args&... args matches the variadic template arguments of std::format and friends.

libcxx/test/std/utilities/format/format.functions/format_to.pass.cpp
69

Good point. I agree it should be a const reference, no idea why I didn't do that initially.

libcxx/test/std/utilities/format/format.functions/vformat_to.locale.pass.cpp
33

Basically when I wrote the tests there was no MAKE_STRING_VIEW macro so I used MAKE_STRING. For P2216 I only need a string_view for the format string, so I made the minimal change required.

That's "Why not std::basic_string_view<CharT> expected as well?" I *think* I can change the expected without any issue too. I'll investigate that.

Mordante updated this revision to Diff 405301.Feb 2 2022, 9:06 AM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante marked 2 inline comments as done.Feb 2 2022, 9:07 AM
ldionne accepted this revision.Feb 4 2022, 2:02 PM
This revision is now accepted and ready to land.Feb 4 2022, 2:02 PM
This revision was automatically updated to reflect the committed changes.
libcxx/test/std/utilities/format/format.functions/formatted_size.pass.cpp