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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


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


================
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
----------------
I'm not sure how to parse this comment. Is something missing here?


================
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);
+}
----------------
These should be "true" and "false", not "1" and "0".


================
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,./<>?"));
+}
----------------
Might be worth testing a string with embedded NUL to make sure that we format until the first NUL rather than the whole string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664