[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline 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>);
----------------
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.


================
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);
+
----------------
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!



================
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>
----------------
`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.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp:80-88
+template <object CharT>
+void test() {
+  test<CharT*>();
+  test<const CharT*>();
+}
+
+int main(int, char**) {
----------------
I strongly recommend flattening these:
```
int main(int, char**) {
  test<char*>();
  test<const char*>();
  test<wchar_t*>();
  test<const wchar_t*>();
}
```
Even if you don't flatten them, at least use different names for `test` (the actual test) and `test` (the thing that calls `test` twice). I don't know our convention off the top of my head, but I know libcxx/test/ has plenty of examples of this to set naming precedent.


================
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);
----------------
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.


================
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);
----------------
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.


================
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());
+}
----------------
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()));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664