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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 10 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/format:351
+template <class _Tp, class _CharT>
+struct _LIBCPP_TEMPLATE_VIS __formatter_char {
+  _LIBCPP_INLINE_VISIBILITY
----------------
vitaut wrote:
> Why add this instead of implementing parse/format in formatter specialization directly?
Not entirely sure what you mean, but this stub class will be removed in D103466.


================
Comment at: libcxx/include/format:632
+  case _CharT(':'):
+    // The arg-id has no format-specifier, advance the input to the format-spec.
+    __parse_ctx.advance_to(__next(__r.first, __end));
----------------
vitaut wrote:
> Looks like the meaning is negated here, this is exactly when we do have format-specifier.
Good catch.


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


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp:66
+
+  test<CharPtr>(STR(" azAZ09,./<>?"), STR("}"), CSTR(" azAZ09,./<>?"));
+}
----------------
vitaut wrote:
> Might be worth testing a string with embedded NUL to make sure that we format until the first NUL rather than the whole string.
I like this suggestion. I also added them to the other string types where the NUL is allowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664