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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114001