[libcxx-commits] [PATCH] D118717: [libc++][format[[nfc] Use string_view in tests.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the review.



================
Comment at: libcxx/test/std/utilities/format/format.functions/format.locale.pass.cpp:40-41
 
-auto test_exception =
-    []<class CharT, class... Args>(std::string_view what, std::basic_string<CharT> fmt, const Args&... args) {
+auto test_exception = []<class CharT, class... Args>(std::string_view what, std::basic_string_view<CharT> fmt,
+                                                     const Args&... args) {
 #ifndef TEST_HAS_NO_EXCEPTIONS
----------------
Quuxplusone wrote:
> 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).
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.


================
Comment at: libcxx/test/std/utilities/format/format.functions/format_to.pass.cpp:69
     assert(false);
   } catch (std::format_error& e) {
     LIBCPP_ASSERT(e.what() == what);
----------------
Quuxplusone wrote:
> 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/
Good point. I agree it should be a const reference, no idea why I didn't do that initially.


================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat_to.locale.pass.cpp:33
 
-auto test = []<class CharT, class... Args>(std::basic_string<CharT> expected, std::basic_string<CharT> fmt,
+auto test = []<class CharT, class... Args>(std::basic_string<CharT> expected, std::basic_string_view<CharT> fmt,
                                            const Args&... args) {
----------------
Quuxplusone wrote:
> Why not `std::basic_string_view<CharT> expected` as well? (Ditto throughout.)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118717/new/

https://reviews.llvm.org/D118717