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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante marked 13 inline comments as done.
Mordante added a comment.

Thanks for the review! I see two major changes to be done:

- Allowing the format-spec to accept the end-of-input as a valid termination.
- Increment the maximum value for width and precision.

These changes will affect the patches depending on this patch.
Before updating this patch I first want to update all affected patches.



================
Comment at: libcxx/include/__format/format_string.h:67
+    __throw_format_error(
+        "Format-spec numeric argument larger than the maximum");
+
----------------
vitaut wrote:
> Is it even possible to have 1 billion arguments to a function? I don't think so and therefore this should probably be an assert instead of an exception.
Good point.


================
Comment at: libcxx/include/__format/format_string.h:91-93
+ * By limiting the maximum to 999.999.999 there's an easier way. Make sure the
+ * length of the input contains no more than 9 characters. In that case we only
+ * need to check against the end of the input.
----------------
vitaut wrote:
> The limit of `999.999.999` is OK for an argument ID but is unnecessarily restrictive for width and precision. {fmt} supports any nonnegative `int` value and so do common `printf` implementations (almost, some of them don't handle `INT_MAX`, but they do handle values >= 1,000,000,000: https://godbolt.org/z/Esb9e6Wxh).
> 
> Having such limit will make migration from `printf` and other formatting facilities more difficult, please remove or make sure that it's only used for argument IDs.
I expected 999.999.999 to be large enough. I still think formatting this number of characters isn't realistic. But since `printf` supports it I agree it would be good to allow this number here. This change will affect more patches depending on this one.


================
Comment at: libcxx/include/__format/format_string.h:111-113
+  if (__begin == __end_input)
+    __throw_format_error(
+        "End of input while parsing format-spec numeric value");
----------------
vitaut wrote:
> I don't think this error is correct. For example, you could have a format-spec consisting of a single width passed to `formatter::parse` function which should handle it without an error.
I see I originally interpretted http://eel.is/c++draft/format#formatter.requirements-2.7 differently.
```
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 }.
```
I thought throwing here was valid, but rereading it, I agree with your interpretation.


================
Comment at: libcxx/include/format:69
+  template<class... Args>
+    string format(string_view fmt, const Args&... args);
+  template<class... Args>
----------------
vitaut wrote:
> `string_view` should be replaced with `format_string<Args...>` per http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2216r3.html but I guess it can be done in a separate diff.
Yes P2216 will be done later.


================
Comment at: libcxx/include/format:779
+            const _Args&... __args) {
+  // TODO FMT Improve PoC: using std::string is inefficient.
+  string __str = vformat(__fmt, _VSTD::make_format_args(__args...));
----------------
vitaut wrote:
> What does PoC stand for?
Proof-of-concept.


================
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:
> I'm not sure how to parse this comment. Is something missing here?
This is part of the standard wording of the part tested.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp:65-66
+
+  test(STR("1"), STR("}"), true);
+  test(STR("0"), STR("}"), false);
+}
----------------
vitaut wrote:
> These should be "true" and "false", not "1" and "0".
Correct, that will be done in D103670. It's part of the commit message that this is a temporary version.


================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:128-130
+  test(STR("hello 42.000000"), STR("hello {}"), static_cast<float>(42));
+  test(STR("hello 42.000000"), STR("hello {}"), static_cast<double>(42));
+  test(STR("hello 42.000000"), STR("hello {}"), static_cast<long double>(42));
----------------
vitaut wrote:
> You probably know that but the default format is wrong. The output should be `42` without decimal point or trailing zeros.
Yes this should be fixed once I add proper floating-point support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664