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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added a comment.

As commented inline, `to_chars` is not a great API for implementing this because you have to do extra work to reparse the output. But it's probably OK for the initial implementation.



================
Comment at: libcxx/benchmarks/formatter_float.bench.cpp:91-92
+    // std::numeric_limits<F>::max() - -std::numeric_limits<F>::max()
+    std::uniform_real_distribution<F> distribution(-std::numeric_limits<F>::max() / F(2.1),
+                                                   std::numeric_limits<F>::max() / F(2.1));
+
----------------
I am not sure if uniform distribution over all FP numbers is very meaningful for benchmarking. The perf of FP formatting methods often depends on the number of significant digits which for these inputs will likely be leaning towards the maximum. A better way of generating inputs can be found in https://github.com/miloyip/dtoa-benchmark.


================
Comment at: libcxx/benchmarks/formatter_float.bench.cpp:200
+struct Precision<PrecisionE::Small> {
+  static constexpr const char* fmt = ".6";
+};
----------------
Note that 6 is the default for fixed, general and scientific so this test is effectively the same as None on these formats.


================
Comment at: libcxx/include/__format/formatter.h:209
+                                   __format_spec::_Flags::_Alignment __alignment, const _CharT* __exponent,
+                                   size_t __trailing_zero) -> decltype(__out_it) {
+  _LIBCPP_ASSERT(__first <= __last, "Not a valid range");
----------------
nit: I'd suggest renaming `__trailing_zero` to something reflecting that it's **the number of** trailing zeros, e.g. `__num_trailing_zeros` or at least `__trailing_zeros`. Right now it looks like a flag indicating whether to write trailing zeros.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:27
+#include <__format/formatter.h>
+#include <__format/formatter_integral.h>
+#include <__utility/move.h>
----------------
What is this for?


================
Comment at: libcxx/include/__format/formatter_floating_point.h:126
+  static constexpr int __max_integral = 308;
+  static constexpr int __max_precision = 1074;
+  static constexpr int __max_precision_value = 4;
----------------
You can do better than that by generating significant digits only which gives maximum precision of 767: https://www.exploringbinary.com/maximum-number-of-decimal-digits-in-binary-floating-point-numbers/. This will require replacing `to_chars` with something more efficient/lower-level and can be done later. This will also avoid dynamic memory allocation.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:144-145
+  // When using a scientific formatting with a precision of 6 a stack buffer
+  // will always suffice. At the moment that isn't important floats and doubles
+  // with a "small" precision will always use a stack buffer.
+  // When supporting long doubles the __max_integral part becomes 4932 which
----------------
There seems to be something missing grammatically in

> At the moment that isn't important floats and doubles
> with a "small" precision will always use a stack buffer.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:238-243
+  __result.__exponent = __find_exponent(__result.__integral, __result.__last);
+
+  // Constrains:
+  // - There's at least one decimal digit before the radix point.
+  // - The radix point, when present, is placed before the exponent.
+  __result.__radix_point = _VSTD::find(__result.__integral + 1, __result.__exponent, '.');
----------------
Switching away from `to_chars` will also get rid of this reparsing overhead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114001