[libcxx-commits] [PATCH] D114001: [libc++][format] Adds formatter floating-point.

Tue Jan 18 10:09:28 PST 2022

Mordante added a comment.

Thanks for the reviews!

================
Comment at: libcxx/include/__format/formatter.h:211
+  _LIBCPP_ASSERT(__first <= __last, "Not a valid range");
+  _LIBCPP_ASSERT(__size < __width, "Precondition failure");
+  _LIBCPP_ASSERT(__num_trailing_zeros > 0, "Use the overload not writing trailing zeros");
----------------
ldionne wrote:
> Why is this an important precondition? Let's write that instead of just telling the user what they already know (that it's a precondition failure).
It was copy pasted and not needed here. I'll remove it.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:99
+// sign                    1 code unit
+// __max_precision_value
+// -----------------------------------
----------------
vitaut wrote:
> vitaut wrote:
> > This looks like the number of digits in the minimum (subnormal) exponent so I suggest renaming accordingly.
> > 
> > Moreover, I don't think you can produce a number that contains both maximum fractional digits and an exponent because for the former you have to use the fixed format which doesn't have an exponent, e.g. (https://godbolt.org/z/jsrPqsEEb)
> > 
> > ```
> >   fmt::print("{:.1074f}", 4.940656458412e-324);
> > ```
> > produces:
> > 
> > ```
> > 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004940656458412465441765687928682213723650598026143247644255856825006755072702087518652998363616359923797965646954457177309266567103559397963987747960107818781263007131903114045278458171678489821036887186360569987307230500063874091535649843873124733972731696151400317153853980741262385655911710266585566867681870395603106249319452715914924553293054565444011274801297099995419319894090804165633245247571478690147267801593552386115501348035264934720193790268107107491703332226844753335720832431936092382893458368060106011506169809753078342277318329247904982524730776375927247874656084778203734469699533647017972677717585125660551199131504891101451037862738167250955837389733598993664809941164205702637090279242767544565229087538682506419718265533447265625
> > ```
> > (note that there is no exponent)
> > 
> > And if you use some other format, you'll get an exponent but no leading zeros so buffer requirements will be lower.
> > 
> > Therefore you can remove exponent, sign and __max_precision_value from these computations and simplify the trait.
> Also you might want to add formatting of the minimum subnormals to tests because it's an interesting corner case and may reveal buffer issues if the test suite is run with sanitizers.
I've already a test for it, I mentioned you in a comment at that place.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:99
+// sign                    1 code unit
+// __max_precision_value
+// -----------------------------------
----------------
Mordante wrote:
> vitaut wrote:
> > vitaut wrote:
> > > This looks like the number of digits in the minimum (subnormal) exponent so I suggest renaming accordingly.
> > > 
> > > Moreover, I don't think you can produce a number that contains both maximum fractional digits and an exponent because for the former you have to use the fixed format which doesn't have an exponent, e.g. (https://godbolt.org/z/jsrPqsEEb)
> > > 
> > > ```
> > >   fmt::print("{:.1074f}", 4.940656458412e-324);
> > > ```
> > > produces:
> > > 
> > > ```
> > > 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004940656458412465441765687928682213723650598026143247644255856825006755072702087518652998363616359923797965646954457177309266567103559397963987747960107818781263007131903114045278458171678489821036887186360569987307230500063874091535649843873124733972731696151400317153853980741262385655911710266585566867681870395603106249319452715914924553293054565444011274801297099995419319894090804165633245247571478690147267801593552386115501348035264934720193790268107107491703332226844753335720832431936092382893458368060106011506169809753078342277318329247904982524730776375927247874656084778203734469699533647017972677717585125660551199131504891101451037862738167250955837389733598993664809941164205702637090279242767544565229087538682506419718265533447265625
> > > ```
> > > (note that there is no exponent)
> > > 
> > > And if you use some other format, you'll get an exponent but no leading zeros so buffer requirements will be lower.
> > > 
> > > Therefore you can remove exponent, sign and __max_precision_value from these computations and simplify the trait.
> > Also you might want to add formatting of the minimum subnormals to tests because it's an interesting corner case and may reveal buffer issues if the test suite is run with sanitizers.
> I've already a test for it, I mentioned you in a comment at that place.
I want to optimize the storage size when looking at the optimization to not store unneeded zeros. Then I'll determine the proper buffer sizes.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:103
+
+// TODO FMT Add long double specialization when to_chars has proper long double support.
+template <class _Tp>
----------------
ldionne wrote:
> Should we file an issue on GitHub for this?
I can do that, probably the same for some other of the TODO's. I'll have a look at it later.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:142
+public:
+  // TODO FMT Improve this constructor to do a better estimate.
+  // When using a scientific formatting with a precision of 6 a stack buffer
----------------
ldionne wrote:
> Would it be ABI breaking to change this after we've shipped it (I'm asking cause it would seem to change the layout of this type)? If so, and if it's actually likely that we'll change that in the future, we might want to allocate from the heap right away to avoid being stuck later on.
The struct already can be used for local and heap storage, so I foresee no ABI break. I expect this code can be moved to the dylib in the future, which will solve all ABI implications.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:194
+  char* __begin_;
+  allocator<char> __alloc_;
+  char __buffer_[_Traits::__stack_buffer_size];
----------------
ldionne wrote:
> Mordante wrote:
> > vitaut wrote:
> > > Do you need to store the allocator? Is it stateful?
> > Fair point, it's the default so it should be stateless.
> Same question -- I would suggest not storing it. Or if we store it, at least use `[[no_unique_address]]`.
I already removed it locally, but not yet uploaded since I had some more comments to address.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:224-227
+    for (; __first != __last - 3; ++__first) {
+      if (*__first == 'e')
+        return __first;
+    }
----------------
ldionne wrote:
> `_VSTD::find(__first, __last - 3, 'e')`? I agree it feels a bit weird to use an algorithm for at most 3 steps, but I guess it makes the code a bit clearer. Not binding, just a suggestion if you agree it's clearer.
I had that originally but wasn't too happy with it. The code would be something like:
```
__first = _VSTD::find(__first, __last - 3, 'e');
if(__first != __last - 3)
  return __first;
```
When there's no match the function should return `__last` and not `__last - 3`.

In general I prefer to use the algorithms, but in this case I like the current code better.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:300
+
+    // When the exponent isn't found, adjusted it to the proper sentinel.
+    if (__result.__exponent == __last)
----------------
vitaut wrote:
> Isn't exponent always produced in hex format?
Yes this seems to be a copy-paste issue. The `_LIBCPP_ASSERT` at line 312 will fail when this isn't the case.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:392
+
+  // The chars_format::general doesn't add trailing zero's. A printf("%#g") option is not available.
+  __buffer.__remove_trailing_zero();
----------------
vitaut wrote:
> zero's -> zeros
> 
> Also I don't understand this comment. What does it have to do with `printf` and what do you mean by not available? Do you mean that `#` is not yet implemented here?
The comment is outdated and can be removed.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:548
+
+    // TODO FMT _Fp should just be _Tp when to_chars has proper long double support.
+    using _Fp = conditional_t<same_as<_Tp, long double>, double, _Tp>;
----------------
ldionne wrote:
> If we had a GitHub issue for `long double` support in `to_chars`, we could reference it here and in the other places. Then we'd only have to grep for the issue number to see everything we can fix once that is done.
That's a fair point. I think it would be good to think about how we want to organize missing features in GitHub first.

================
Comment at: libcxx/include/__format/formatter_floating_point.h:552
+    __float_buffer<_Fp> __buffer(__has_precision ? int(this->__precision) : -1);
+    __float_result __result = __format_buffer(__buffer, __value, __negative, __has_precision);
+
----------------
vitaut wrote:
> You might want to outline the rest of this function because it doesn't depend on `_Tp`.
It depends on `_Fp` in the buffer. Since `_Fp` is only temporary that it doesn't depend on `_Tp`. So the benefit would only be temporary. So I prefer to keep it as is for now.

================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.context/format.formatter.spec/formatter.floating_point.pass.cpp:427
+  test_value<CharT>(A(0.0));
+  test_value<CharT>(std::numeric_limits<A>::denorm_min());
+  test_value<CharT>(A(1.0));
----------------
@vitaut The subnormal value is tested here.

================
Comment at: libcxx/test/std/utilities/format/format.functions/locale-specific_form.pass.cpp:943
+  test(STR("$-1.234560P+3$$"), en_US, STR("{:$^15.6LA}"), F(-0x1.23456p3));
+  test(STR("-0001.234560P+3"), en_US, STR("{:015.6LA}"), F(-0x1.23456p3));
+
----------------
vitaut wrote:
> Do you test that hex digits are capitalized with `A` somewhere?
I see I don't added them.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114001