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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/include/__format/buffer.h:295
+  __internal_storage<_CharT> __storage_;
+  __output_buffer<_CharT> __output_{__storage_.begin(), __storage_.capacity(), this};
+  typename __writer_selector<_OutIt, _CharT>::type __writer_;
----------------
ldionne wrote:
> It strikes me that perhaps `__internal_storage::capacity()` should be renamed to `__internal_storage::size()`. `capacity()` makes it look like it's not something we can write to (like `std::vector::capacity()`). No action for this patch, and this is just a suggestion.
I see your point, but to bike-shed I'm not too fond of `size()` since the storage doesn't really keep track of the number of elements stored. I think, maybe remove the function and make `__buffer_size_` public instead. I'll look at that in a followup patch.


================
Comment at: libcxx/include/__format/buffer.h:315
+  _LIBCPP_HIDE_FROM_ABI explicit __format_to_n_buffer_base(_OutIt __out_it, _Size __n)
+      : __output_(_VSTD::__unwrap_iter(__out_it), __n, this), __writer_(_VSTD::move(__out_it)) {
+    if (__n <= 0) [[unlikely]]
----------------
ldionne wrote:
> Why do we `unwrap_iter` here? If the iterator is something that e.g. checks the bounds, it seems like it would be useful to keep it wrapped to get this checking?
Unwrapping gives a raw pointer, which allows certain optimizations. For example when using `std::string::iterator`. This is only done when the checks are disabled. The concept on line 119 is used to select this `__format_to_n_buffer_base` specialization.
```
template <class _OutIt, class _CharT>
concept __enable_direct_output = __formatter::__char_type<_CharT> &&
    (same_as<_OutIt, _CharT*>
#if _LIBCPP_DEBUG_LEVEL < 2
     || same_as<_OutIt, __wrap_iter<_CharT*>>
#endif
    );
```
When this concept isn't true `__out_it` will not be unwrapped.


================
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) {
----------------
philnik wrote:
> ldionne wrote:
> > vitaut wrote:
> > > Mordante wrote:
> > > > vitaut wrote:
> > > > > Mordante wrote:
> > > > > > vitaut wrote:
> > > > > > > Mordante wrote:
> > > > > > > > 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.)
> > > > > > > Functions like this should be inlined, otherwise you'll get a new instantiation for every call with different argument types which is a disaster. Compilers are terrible at dealing with this sort of things.
> > > > > > Thanks for the additional information. I've added a TODO to investigate this and when needed file a bug report.
> > > > > If you are referring to my comment then there is no bug to report. I was mostly explaining why this function must be forced inlined. A compiler doesn't have information to figure it out on its own.
> > > > No it's a TODO for me. In general we dislike to add compiler work-arounds in our code. If we need it we often file a bug report, where it hopefully gets fixed.
> > > I would argue that it's not a workaround but a part of the design of `std::format` and the whole reason why we have `v*` functions in the first place.
> > I'm swayed by @vitaut's explanation. `_LIBCPP_ALWAYS_INLINE` is definitely reasonable here then, IMO.
> Could you add a comment explaining the `_LIBCPP_ALWAYS_INLINE`? We've got a few of these around the code base and most of them look to me like they don't actually belong there. I think it would be nice if we could have the reasoning right in the code and not have to hunt it down (assuming there is one to find). I didn't feel like looking into the other `_LIBCPP_ALWAYS_INLINE` yet, but I want to do that at some point and potentially remove some of them.
I already added that to my todo list after I read Louis' comment yesterday ;-) However it seems I already added a comment :-)
I have moved it to the first occurrence (only once since I see no value in duplicating the comment).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110499