[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:13-17
+// For each charT, for each cv-unqualified arithmetic type ArithmeticT other
+// than char, wchar_t, char8_t, char16_t, or char32_t, a specialization
+// - a specialization template<> struct formatter<ArithmeticT, charT>;
+// Implemants ArithmeticT is
+// - bool
----------------
vitaut wrote:
> Mordante wrote:
> > vitaut wrote:
> > > I'm not sure how to parse this comment. Is something missing here?
> > This is part of the standard wording of the part tested.
> The confusing to me part is that "a specialization" appears twice. I suggest replacing `a specialization template<> struct formatter<ArithmeticT, charT>;` with `template<> struct formatter<ArithmeticT, charT>;`
Not to mention, "Implemants" on line 16 definitely isn't standard wording, and line 17 seems to be starting a bulleted list with only one item in it. I think this comment is meant to refer to https://eel.is/c++draft/format.formatter.spec#2.3 and so it should say:

```
// [format.formatter.spec]:
// Each header that declares the template `formatter` provides the following enabled specializations:
// For each `charT`, for each cv-unqualified arithmetic type `ArithmeticT` other than
// char, wchar_­t, char8_­t, char16_­t, or char32_­t, a specialization
//    template<> struct formatter<ArithmeticT, charT>
//
// This file tests with `ArithmeticT=bool`, for each valid `charT`.
```


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


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:59-62
+    const CharT* (F::*parse)(PC&) = &F::parse;
+    assert(parse);
+    Out (F::*format)(bool, C&) = &F::format;
+    assert(format);
----------------
These asserts seem non-standard: I mean, I would by default assume there's no legal guarantee that `&F::parse` compiles at all. `parse` might be a template or overload set.
@vitaut , what's your understanding and/or intent here? Are user-programmers supposed to be able to take member pointers to the `parse` and `format` callables like this? (I hope not!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664