[libcxx-commits] [PATCH] D110499: [libc++][format][5/6] Improve format_to_n.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__format/buffer.h:290
+template <class _OutIt, __formatter::__char_type _CharT, bool>
+requires(output_iterator<_OutIt, const _CharT&>) struct _LIBCPP_TEMPLATE_VIS
+    __format_to_n_buffer_base {
----------------
This formatting strikes me as really strange. I'd expect something like:

```
template <class _OutIt, __formatter::__char_type _CharT, bool>
  requires output_iterator<_OutIt, const _CharT&>
struct _LIBCPP_TEMPLATE_VIS __format_to_n_buffer_base {
```

The current formatting for this declaration is somewhat difficult to read IMO.


================
Comment at: libcxx/include/format:372
+                                _VSTD::__format_context_create(__buffer.make_output_iterator(), __args));
+  return _VSTD::move(__buffer).result();
+}
----------------
You probably want to use `std::` instead of `_VSTD::`


================
Comment at: libcxx/include/format:519
 template <output_iterator<const wchar_t&> _OutIt, class... _Args>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT format_to_n_result<_OutIt>
-format_to_n(_OutIt __out_it, iter_difference_t<_OutIt> __n, locale __loc,
-            wstring_view __fmt, const _Args&... __args) {
-  // TODO FMT Improve PoC: using std::string is inefficient.
-  wstring __str = _VSTD::vformat(_VSTD::move(__loc), __fmt,
-                                 _VSTD::make_wformat_args(__args...));
-  iter_difference_t<_OutIt> __s = __str.size();
-  iter_difference_t<_OutIt> __m =
-      _VSTD::clamp(__n, iter_difference_t<_OutIt>(0), __s);
-  __out_it = _VSTD::copy_n(__str.begin(), __m, _VSTD::move(__out_it));
-  return {_VSTD::move(__out_it), __s};
+_LIBCPP_ALWAYS_INLINE _LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT format_to_n_result<_OutIt>
+format_to_n(_OutIt __out_it, iter_difference_t<_OutIt> __n, locale __loc, wstring_view __fmt, const _Args&... __args) {
----------------
I would rather not use `_LIBCPP_ALWAYS_INLINE` I would let the compiler make the right call here -- `_LIBCPP_ALWAYS_INLINE` has a negative impact on various optimizations and on the debugging experience, so we should steer clear of it unless we have a specific reason to. A function being a one-liner isn't a sufficient reason -- the optimizer should be able to figure out whether it is worth actually generating a function call (most likely not).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110499