[libcxx-commits] [PATCH] D103433: [libc++][format] Adds integer formatter.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut requested changes to this revision.
vitaut added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__format/formatter_integer.h:111-116
+    using _To = long long;
+    if (__value < numeric_limits<_To>::min() ||
+        __value > numeric_limits<_To>::max())
+      __throw_format_error("128 bit value is outside of implemented range");
+
+    return _Base::format(static_cast<_To>(__value), __ctx);
----------------
AFAICS `to_chars` supports 128-bit integers (https://godbolt.org/z/7GTPrcq88) or am I missing something?


================
Comment at: libcxx/include/__format/formatter_integral.h:71
+ *   Note the boolean string format-type isn't supported in this class.
+ * * A typedef C++-type group combing the @ref __formatter_integral with a
+ *   parser:
----------------
Did you mean "combining"?


================
Comment at: libcxx/include/__format/formatter_integral.h:88
+                          __parser_locale_specific_form> {
+public:
+  /**
----------------
Why not make these protected to keep the public API of `formatter` specializations clean?


================
Comment at: libcxx/include/__format/formatter_integral.h:136
+    if (this->__alignment == _Flags::_Alignment::__default)
+      this->__alignment = _Flags::_Alignment::__left;
+  }
----------------
Here and elsewhere the default alignment should depend on the presentation type: http://eel.is/c++draft/format#tab:format.align

i.e. `format("{:10}", true)` is left-aligned while `format("{:10d}", true)` is right-aligned.


================
Comment at: libcxx/include/__format/formatter_integral.h:231
+
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+/**
----------------
`#ifdef` can be omitted because there is nothing that use `std::locale` in `__determine_grouping`. It will just be unused if locales are disabled.


================
Comment at: libcxx/include/__format/formatter_integral.h:304
+      if (__value < 0) {
+        __negative = true;
+        __r = __complement(__r);
----------------
Why store `__negative` instead of passing it as an argument to `__format_unsigned_integral`?


================
Comment at: libcxx/include/__format/formatter_integral.h:314
+  [[nodiscard]] _LIBCPP_INLINE_VISIBILITY auto
+  __format_char(integral auto __value, auto& __ctx) -> decltype(__ctx.out()) {
+    if (this->__alignment == _Flags::_Alignment::__default)
----------------
nit: I'd rename it to `__format_as_char` because `__value` is not a char but an integer which we convert and format as char.


================
Comment at: libcxx/include/__format/formatter_integral.h:315-316
+  __format_char(integral auto __value, auto& __ctx) -> decltype(__ctx.out()) {
+    if (this->__alignment == _Flags::_Alignment::__default)
+      this->__alignment = _Flags::_Alignment::__right;
+
----------------
Isn't the alignment supposed to be resolved at parse time by calling `__handle_char` / `__handle_bool`?


================
Comment at: libcxx/include/__format/formatter_integral.h:329
+        if (__value >
+            static_cast<make_signed_t<_CharT>>(numeric_limits<_CharT>::max()))
+          __throw_format_error(
----------------
`_CharT` is already signed so `make_signed_t` doesn't do anything. Did you mean `make_unsigned_t`?


================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:392-393
+  // *** align-fill & width ***
+  // TODO FMT Validate whether this should be left aligned
+  test(STR("answer is '     *'"), STR("answer is '{:6c}'"), I(42));
+  test(STR("answer is '     *'"), STR("answer is '{:>6c}'"), I(42));
----------------
Per my earlier comment this should indeed be left-aligned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433