[libcxx-commits] [PATCH] D112017: [libcxx][format] Fix how we handle char traits in formatter<string> and formatter<string_view>

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxx/include/__format/formatter_string.h:147
+
+  _LIBCPP_HIDE_FROM_ABI auto format(const basic_string_view<_CharT, _Traits>& __str, auto& __ctx)
+      -> decltype(__ctx.out()) {
----------------
Quuxplusone wrote:
> Style: Linebreak after `auto`, please.
> 
> Perf: Shouldn't `basic_string_view<_CharT, _Traits> __str` be passed by value, not by const-ref? Or is there a subtle reason we're passing by const-ref here in particular?
I believe `formatter<basic_string_view<_CharT, _Traits>, _CharT>::format` needs to meet the [Formatter named requirements](https://en.cppreference.com/w/cpp/named_req/Formatter), which limits what the function signature can be.


================
Comment at: libcxx/include/__format/formatter_string.h:134
       -> decltype(__ctx.out()) {
-    return _Base::format(_VSTD::basic_string_view<_CharT>(__str), __ctx);
+    return _Base::format(_VSTD::basic_string_view<_CharT, _Traits>(__str), __ctx);
   }
----------------
Quuxplusone wrote:
> Mordante wrote:
> > I think we can here directly convert to a `VSTD::basic_string_view<_CharT>` and drop the `_Traits`.
> We can also drop the `_VSTD::` (on non-functions). I wonder if we could make this self-documenting (i.e. eliminate the comment) by doing something like
> ```
> using _StringView = basic_string_view<_CharT, char_traits<_CharT>>;
> return _Base::format(_StringView(__str.data(), __str.size()), __ctx);
> ```
> but meh, it seems about the same to me.
Yeah, unfortunately, I don't see any good way of highlighting the change of _Traits. Since they're about the same I think it's probably better not to introduce a second source of truth by explicitly specifying the `_Traits` template argument to be `char_traits<_CharT>`. Especially since we rely on the default for the `__str` parameter in the definition of `__formatter_string::format`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112017