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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:38
+  static_assert(std::semiregular<decltype(formatter)>);
+  formatter.parse(parse_ctx);
+
----------------
Mordante wrote:
> vitaut wrote:
> > I'd add a check that the return value points to the end of the format string here and elsewhere.
> AFAIK this doesn't need to point to the end of the format string. Based on earlier comments and wording of http://eel.is/c++draft/format#tab:formatter
> ```
> Parses format-spec ([format.string]) for type T in the range [pc.begin(), pc.end()) until the first unmatched character.
> Throws format_­error unless the whole range is parsed or the unmatched character is }.
> [Note 1: This allows formatters to emit meaningful error messages.
> — end note]
> Stores the parsed format specifiers in *this and returns an iterator past the end of the parsed range.
> ```
> I made changes to this function. It now either returns an iterator to `fmt.end()` or an iterator at the closing `}` of the replacement-field.
Good point.


================
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:
> > 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));
> ```
> is this really the most ergonomic way to fit all these puzzle pieces together?

Looks OK to me since it's only a test . These are rarely used in user code, mostly when reusing formatters which is much easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664