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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
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);
----------------
vitaut wrote:
> AFAICS `to_chars` supports 128-bit integers (https://godbolt.org/z/7GTPrcq88) or am I missing something?
libc++ accepts 128-bit integers, but silently truncates them to 64-bit. IMO this is a bug and it's on my TODO to fix this.
It's visible in the generated assembly, it contains a`std::__1::__itoa::__pow10_64` lookup table instead of a `std::__1::__itoa::__pow10_128`.


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


================
Comment at: libcxx/include/__format/formatter_integral.h:136
+    if (this->__alignment == _Flags::_Alignment::__default)
+      this->__alignment = _Flags::_Alignment::__left;
+  }
----------------
vitaut wrote:
> 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.
This function is only called when a char is used for the char representation is selected.
Only the `__handle_integer` is used slightly different. That was basically done to use different formatting for
`format("{:6c}", 42)` and `format("{:6}", '*')`. But I'll adjust `__handle_integer`.

See my response to https://reviews.llvm.org/D103433/new/#inline-1010793 for more information.




================
Comment at: libcxx/include/__format/formatter_integral.h:304
+      if (__value < 0) {
+        __negative = true;
+        __r = __complement(__r);
----------------
vitaut wrote:
> Why store `__negative` instead of passing it as an argument to `__format_unsigned_integral`?
Basically to avoid passing an argument to a forwarding function. IMO both are a valid design choice. But as you mention in another comment "Among other things this would inhibit format string compilation in the future. Please remove.". So I'll apply your suggestion.


================
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;
+
----------------
vitaut wrote:
> Isn't the alignment supposed to be resolved at parse time by calling `__handle_char` / `__handle_bool`?
Only partly, not for integers formatted as char. But I'll adjust this.

See my response to https://reviews.llvm.org/D103433/new/#inline-1010793 for more information.


================
Comment at: libcxx/include/__format/formatter_integral.h:329
+        if (__value >
+            static_cast<make_signed_t<_CharT>>(numeric_limits<_CharT>::max()))
+          __throw_format_error(
----------------
vitaut wrote:
> `_CharT` is already signed so `make_signed_t` doesn't do anything. Did you mean `make_unsigned_t`?
Should indeed be `make_unsigned_t`. Odd the compiler doesn't complain about a useless cast.


================
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));
----------------
vitaut wrote:
> Per my earlier comment this should indeed be left-aligned.
I assumed that was intended. However my interpretation of the wording requires it to be right aligned:
`This is the default for arithmetic types other than charT and bool or when an integer presentation type is specified.`
My interpretation is that the supplied type in an aritmetic type, so this rule applies.
Do you agree with my interpretation?

I agree it's silly that the result of `format("{:6c}", 42);` differs from the result of `format("{:6}", '*');`. So I'll adjust it.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433