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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


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


================
Comment at: libcxx/include/__format/formatter.h:212
+  _LIBCPP_ASSERT(__size < __width, "Precondition failure");
+  _LIBCPP_ASSERT(__num_trailing_zeros > 0, "Use the overload not writing trailing zeros");
+
----------------
This should be worded not to imply that the user is the one "using" that overload. If it ever shows up in a user's crash report, it would most likely be indicative of a libc++ bug, not something on their end. I think `The overload not writing trailing zeros should have been used` would be better.


================
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>
----------------
Should we file an issue on GitHub for this?


================
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
----------------
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.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:182-187
+  char* begin() const { return __begin_; }
+  char* end() const { return __begin_ + __size_; }
+
+  int __precision() const { return __precision_; }
+  int __num_trailing_zeros() const { return __trailing_zero_; }
+  void __remove_trailing_zero() { __trailing_zero_ = 0; }
----------------
`_LIBCPP_HIDE_FROM_ABI` throughout.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:194
+  char* __begin_;
+  allocator<char> __alloc_;
+  char __buffer_[_Traits::__stack_buffer_size];
----------------
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]]`.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:224-227
+    for (; __first != __last - 3; ++__first) {
+      if (*__first == 'e')
+        return __first;
+    }
----------------
`_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.


================
Comment at: libcxx/include/__format/formatter_floating_point.h:360
+                                                                           char* __integral) {
+  __float_result __result = __format_buffer_scientific_lower_case(__buffer, __value, __precision, __integral);
+  *__result.__exponent = 'E';
----------------
This needs to be qualified. I haven't paid attention elsewhere, but I suggest you take a quick look at all your function calls and qualify them when necessary (or all the time so that we don't even have to ask ourselves).


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


================
Comment at: libcxx/include/__format/formatter_floating_point.h:659-660
+  // TODO FMT _Fp should just be _Tp when to_chars has proper long double support.
+  _LIBCPP_HIDE_FROM_ABI __float_result __format_buffer(__float_buffer<_Fp>& __buffer, _Tp __value, bool __negative,
+                                                       bool __has_precision) {
+    char* __first = __insert_sign(__buffer.begin(), __negative, this->__sign);
----------------
vitaut wrote:
> Mordante wrote:
> > vitaut wrote:
> > > This and almost all other function template here don't need to be defined in a header because they are only instantiated for a few known types (float/double/long double). Moving them out of the header would be beneficial for build times (esp. since you can also move includes).
> > This and other code could be moved to the dylib. There's one big disadvantage of the dylib. As soon as the function is exported in the dylib its ABI is frozen. As long as the code is still under development I don't want to freeze the ABI. Especially this function will change once we have proper long double support. Moving code to the dylib is on my todo list, but I want to wait until the code has matured a bit further.
> > 
> > For example the `this->__sign` on the next line will change when the formatter no longer inherits from a parser. That will be impossible when I move it to the dylib now.
> > Moving code to the dylib is on my todo list, but I want to wait until the code has matured a bit further.
> 
> Sounds reasonable. Thanks for clarifying.
Indeed, FWIW I support keeping everything in the headers and marked with `_LIBCPP_HIDE_FROM_ABI` for the time being, since that means we don't have to commit to keeping these implementation detail functions around forever (which is the case when we move them to the dylib). Once everything is stable, we can "outline" them to the dylib as a size optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114001