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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante marked 2 inline comments as done.
Mordante added a comment.

Thanks for the review! I'll address the comment shortly.



================
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 {
----------------
ldionne wrote:
> 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.
I tend to use clang-format and that's what clang-format made of it. I've filed some bug reports. I'm quite sure ToT clang-format will format it as you suggested.


================
Comment at: libcxx/include/format:372
+                                _VSTD::__format_context_create(__buffer.make_output_iterator(), __args));
+  return _VSTD::move(__buffer).result();
+}
----------------
ldionne wrote:
> You probably want to use `std::` instead of `_VSTD::`
Actually, for consistency,  I prefer to use `_VSTD::`. This patch predates the changes to use `std::`. On my TODO list I have an item to do `s/_VSTD/std` for all format related code.


================
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) {
----------------
ldionne wrote:
> 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).
This was suggested by @vitaut. I don't mind to remove it. I still want to look closer at the codegen after P2216 and P2418 are done. (I already identified code bloat in the parser code, which is on my list for P2216 and P2418.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110499