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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 7 inline comments as done.
Mordante added a comment.

Thanks a lot for your review!



================
Comment at: libcxx/include/__format/format_string.h:99-100
+__parse_number(const _CharT* __begin, const _CharT* __end_input) {
+  _LIBCPP_ASSERT(__format::__number_max == INT32_MAX,
+                 "The algorithm is implemented based on this value.");
+  /*
----------------
vitaut wrote:
> This could be a static assert.
Good catch!


================
Comment at: libcxx/include/format:687
+        __throw_format_error(
+            "The format string contains an invalid escape sequence");
+
----------------
vitaut wrote:
> I would call it something like "unmatched '}'" rather than "an invalid escape sequence" because we don't know whether the intent was to have an escape sequence.
I like your reasoning, I've changed it.


================
Comment at: libcxx/include/format:706
+  return __format::__vformat_to(
+      basic_format_parse_context{__fmt, __args.__size()},
+      basic_format_context{_VSTD::move(__out_it), __args});
----------------
vitaut wrote:
> I don't think you need to pass `__args.__size()` here because it is for compile-time checks only.
But doesn't that mean I need to add it again when I start working on P2216?


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


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:51-52
+void test() {
+  test(STR("1"), STR("}"), true);
+  test(STR("0"), STR("}"), false);
+}
----------------
vitaut wrote:
> I'd test that empty string is also parsed correctly which is an important corner case.
In other patches, for example` D103433. I added an extra layer to test both "}" and "" from one tester. I applied the same method to the tests in this directory. I also changed the names of the `test` functions to not use the overload resolution to pick the proper function. When we're happy with that approach I'll use these improvements for later patches in the series.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.floating_point.pass.cpp:58
+  using A = Arithmetic;
+  test(STR("{}"), A(-std::numeric_limits<float>::max()));
+  test(STR("{}"), A(-std::numeric_limits<float>::min()));
----------------
vitaut wrote:
> Why do you have "}" in other tests and "{}" here?
This should also be "}", this was an older approach. The floats haven't had as much love as the other implemented types yet. (Mainly since for floats I still need to get `std::to_chars` working properly.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664