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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM, so I'll be the second approver here. But I'd like to see both of my comments on `formatter<basic_string_view>::format` addressed.



================
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()) {
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112017