[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jul 6 11:09:44 PDT 2021
Mordante marked 7 inline comments as done.
Mordante added a comment.
Thanks for all the review comments!
================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:52-55
+ static_assert(std::is_copy_constructible_v<F>);
+ static_assert(std::is_move_constructible_v<F>);
+
+ static_assert(std::is_swappable_v<F>);
----------------
Quuxplusone wrote:
> This was a vestige of the time when the C++20 concepts weren't in trunk yet, right?
> I believe every instance of this pattern should be replaced with a one-liner:
> ```
> static_assert(std::semiregular<std::formatter<bool, CharT>>);
> test(...
> ```
> In fact, consider moving the assertion up into `test`, where you already have a variable of the correct type, so you could just say
> ```
> static_assert(std::semiregular<decltype(formatter)>);
> ```
> and wouldn't have to worry about typos potentially introduced by faulty cut-and-paste.
> This was a vestige of the time when the C++20 concepts weren't in trunk yet, right?
Correct!
> I believe every instance of this pattern should be replaced with a one-liner:
> ```
> static_assert(std::semiregular<std::formatter<bool, CharT>>);
> test(...
> ```
Good point, this indeed seems to be the case.
> In fact, consider moving the assertion up into `test`, where you already have a variable of the correct type, so you could just say
> ```
> static_assert(std::semiregular<decltype(formatter)>);
> ```
> and wouldn't have to worry about typos potentially introduced by faulty cut-and-paste.
This seems to be the only one using a hard-coded type instead of template arguments. But avoiding possible copy-paste issues always is a good thing(tm). Especially now the four `static_asserts` can be replaced by just one.
================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:29-43
+template <class CharT, class A>
+void test(std::basic_string<CharT> expected, std::basic_string<CharT> fmt,
+ A arg) {
+ std::basic_format_parse_context<CharT> parse_ctx{fmt};
+ std::formatter<bool, CharT> formatter;
+ formatter.parse(parse_ctx);
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > I'd find this easier to read:
> > > ```
> > > template <class StringT, class ArithmeticT>
> > > void test(StringT expected, StringT fmt, ArithmeticT arg) {
> > > using CharT = typename StringT::value_type;
> > > auto parse_ctx = std::basic_format_parse_context<CharT>(fmt);
> > > std::formatter<bool, CharT> formatter;
> > > formatter.parse(parse_ctx);
> > >
> > > StringT result;
> > > auto out = std::back_inserter(result);
> > > using FormatCtxT = std::basic_format_context<decltype(out), CharT>;
> > >
> > > auto format_ctx = FormatCtxT(out, std::make_format_args<FormatCtxT>(arg));
> > > formatter.format(arg, format_ctx);
> > > assert(result == expected);
> > > }
> > > ```
> > > And then, if `ArithmeticT` is always `bool`, I strongly recommend making the substitution. Otherwise, you're implicitly relying on template type deduction down in `main` to deduce `bool` from `true` and `false` — which is obviously not-too-bad in this case, but you're going to have to use a different strategy once you reach the formatter for `short`. So I recommend switching strategies right now, in order to be consistent across all the new test files.
> > > ```
> > > template <class StringT>
> > > void test(StringT expected, StringT fmt, bool arg) {
> > > ```
> > > (and then the rest as above)
> > I used most of this suggestion and updated the other tests as well. In this case I prefer to not add another layer of indirection, instead I replaced the template argument `ArithmeticT` with a `bool`.
> LGTM! Btw, @vitaut, is this really the most ergonomic way to fit all these puzzle pieces together? I know this isn't the "expected way to use `<format>`," because the expected way is just `std::format("{}", arg)`... but this code feels very loop-de-loopy even for internal implementation stuff.
>
> std::formatter<bool, CharT> formatter;
> // ok cool so this knows how to format bools?
> auto format_ctx = FormatCtxT(out, std::make_format_args<FormatCtxT>(arg));
> // we're passing in `arg` here, so now it's formatted?
> formatter.format(arg, format_ctx);
> // no, ok, pass in `arg` AGAIN, third time's the charm!
>
I agree this example is quite verbose, but in the format function it isn't that ugly. There the parsing and formatting for an argument looks like
```
formatter<decltype(__arg), _CharT> __formatter;
__parse_ctx.advance_to(__formatter.parse(__parse_ctx));
__ctx.advance_to(__formatter.format(__arg, __ctx));
```
================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp:47-55
+template <class T>
+concept pointer = std::is_pointer_v<T>;
+
+template <class T>
+concept object = !pointer<T>;
+
+template <pointer CharPtr>
----------------
Quuxplusone wrote:
> `pointer` is rightly named but unnecessary; you could just `requires is_pointer_v<CharPtr>` (or in fact do nothing at all, because this function doesn't need to be constrained for any technical reason). `object` is misnamed, since it is significantly different from `std::is_object_v` (but also, it's unused). Write instead:
> ```
> template<class CStringT>
> void test() {
> using CharT = std::remove_cv_t<std::remove_pointer_t<CStringT>>;
> // alternatively, = std::iter_value_t<CStringT>;
> static_assert(std::semiregular<std::formatter<CStringT, CharT>>);
> test<CStringT>(STR(" azAZ09,./<>?"), STR("}"), CSTR(" azAZ09,./<>?"));
>
> std::basic_string<CharT> s(CSTR("abc\0abc"), 7);
> test<CStringT>(STR("abc"), STR("}"), s.c_str());
> }
> ```
> I think what I've got for the embedded-NUL test ought to work. I have not tried it.
> `pointer` is rightly named but unnecessary; you could just `requires is_pointer_v<CharPtr>` (or in fact do nothing at all, because this function doesn't need to be constrained for any technical reason). `object` is misnamed, since it is significantly different from `std::is_object_v`
This point is no longer needed, since I flattened the tests as you suggested in another comment.
> (but also, it's unused). Write instead:
> ```
> template<class CStringT>
> void test() {
> using CharT = std::remove_cv_t<std::remove_pointer_t<CStringT>>;
> // alternatively, = std::iter_value_t<CStringT>;
> static_assert(std::semiregular<std::formatter<CStringT, CharT>>);
This one is move to the real tester using the `decltype` as you suggested.
> test<CStringT>(STR(" azAZ09,./<>?"), STR("}"), CSTR(" azAZ09,./<>?"));
>
> std::basic_string<CharT> s(CSTR("abc\0abc"), 7);
> test<CStringT>(STR("abc"), STR("}"), s.c_str());
> }
> ```
> I think what I've got for the embedded-NUL test ought to work. I have not tried it.
That's actually a nice looking method, thanks for the suggestion!
I tested it and it passes the tests locally.
================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.const_char_array.pass.cpp:55-60
+ std::basic_string<CharT> buffer{text, text + N};
+ // Note not too found of this hack
+ Str* data = reinterpret_cast<Str*>(buffer.c_str());
+
+ Ctx ctx{std::back_inserter(result), std::make_format_args<Ctx>(*data)};
+ formatter.format(*data, ctx);
----------------
Quuxplusone wrote:
> I think what you want here is
> ```
> std::formatter<ArrayT, CharT> formatter;
> [...]
> CharT array[N];
> std::copy(text, text + N, array);
> ArrayT& arg = array;
> auto format_ctx = FormatCtxT(std::back_inserter(result), std::make_format_args<FormatCtxT>(arg));
> formatter.format(arg, ctx);
> ```
> Note consistent use of the names `FormatCtxT`, `arg`, etc. among all the test cases.
> I admit I don't understand why you're doing the weird thing with the class-type NTTP `Tester` here; couldn't you just hard-code `N` in the test case? We don't need to "deduce the size of the input" when there //is// only one input. And then you can get rid of a lot of "magic" — no more CTAD/CNTTPs, no more `__builtin_memcpy`, etc.
The formatter type is `template<size_t N> struct formatter<const charT[N], charT>;` The size of `N` isn't fixed in this test and future tests in the series add more sizes. Since this works I prefer to keep it like this.
I can have a look at the consistency, but the tests already have some minor differences between them.
================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:27-32
+ // ** Test invalid format strings ***
+ test_exception("The format string terminates while parsing a replacement "
+ "field or an escape sequence",
+ STR("{"));
+ test_exception("The replacement field misses the terminating '}'", STR("{:"),
+ 42);
----------------
Quuxplusone wrote:
> IIUC, these tests are testing the wording of libc++-specific `what`-strings that are not mandated by the Standard, is that right? If so, they belong in libcxx/test/libcxx/.
> Or perhaps it would be better to leave the tests here, but put every instance of `assert(e.what() == what)` under a libc++-specific `#ifdef`. That way, on non-libc++ libraries, we'd test that //some// exception was correctly thrown; we just wouldn't test its what-string.
I guarded them with `#ifdef _LIBCPP_VERSION`. I prefer to keep them since they've been a great aid during debugging and verification.
================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat_to.pass.cpp:36-41
+template <class CharT>
+bool operator==(const std::list<CharT>& lhs,
+ const std::basic_string<CharT>& rhs) {
+ return lhs.size() == rhs.size() &&
+ std::equal(lhs.begin(), lhs.end(), rhs.begin());
+}
----------------
Quuxplusone wrote:
> Please don't provide `==`s for library types, not even in .cpp files. In this case your initial `size` check can be omitted anyway, because `std::equal` can do it: just change e.g. line 68 to
> ```
> assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
> ```
I'm not objecting against this change, but what's the reason you done like `operator==` for library types?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96664/new/
https://reviews.llvm.org/D96664
More information about the libcxx-commits
mailing list