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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.

LGTM!



================
Comment at: libcxx/include/__format/formatter_string.h:148
+  _LIBCPP_HIDE_FROM_ABI auto
+  format(basic_string_view<_CharT, _Traits> __str, auto& __ctx)
+      -> decltype(__ctx.out()) {
----------------
Quuxplusone wrote:
> Aargh, I was gonna say "ship it" and then I noticed that... none of these `format` methods are marked `const`?! What's up with that?
> https://en.cppreference.com/w/cpp/named_req/Formatter says:
> > `formatter.format(arg, format_context)` [should work with] `formatter`, a **value** of type `Formatter`
> 
> What does "value" mean in this context? It probably doesn't mean "lvalue" or it would have said "lvalue," right? I suspect it means "(possibly const) lvalue or rvalue," in which case `format` should always be const-qualified. But then how does any of this //ever// work, today, given that we're not const-qualifying these `format` methods? Does this suddenly need further investigation?
> 
> Ping @Mordante — clarification please? (If @Mordante says "yeah this is all working as intended," then you can land this AFAIC.)
I'm not entirely seeing what exactly the issue is. But there's no requirement the format member function is a `const` member function. 
http://eel.is/c++draft/format#formatter.spec-6 contains a sample of a user defined formatter, which has a non-`const` member function.

When a formatter is used it:
- first calls the parse member function, this function should store the state of the parsing, so shouldn't be `const`,
- then calls format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112017