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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
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:
> vitaut wrote:
> > DanielMcIntosh-IBM wrote:
> > > vitaut wrote:
> > > > Quuxplusone wrote:
> > > > > Mordante wrote:
> > > > > > 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.
> > > > > > http://eel.is/c++draft/format#formatter.spec-6 contains a sample of a user defined formatter, which has a non-const member function.
> > > > > 
> > > > > Okay, works for me!
> > > > > 
> > > > > FWIW: Sounds like the situation is analogous to how it works with the distributions from <random>: some of the <random> distributions are stateless and thus could //physically// be `const`-qualified, but that would actually be worse QoI because it would harm portability-to-other-vendors and/or refactorability-to-other-distribution-types-that-happen-to-be-stateful.
> > > > I'd recommend marking `format` functions as const even though it's not required because they shouldn't mutate the `formatter` state and it will help with format string compilation in the future.
> > > I'm going to land the changes for now.
> > > Changing the `format` functions to const should probably be done across the board if it's going to be done at all, so that should probably go in a separate patch anyways.
> > Maybe we should even open an LWG issue clarifying that all standard formatters have a const format function.
> @vitaut wrote:
> > `format` functions [...] shouldn't mutate the `formatter` state
> 
> @Mordante wrote:
> > http://eel.is/c++draft/format#formatter.spec-6 contains a sample of a user-defined formatter, which has a non-const member function.
> 
> @vitaut wrote:
> > Maybe we should even open an LWG issue clarifying that all [...] formatters [must] have a const `format` function.
> 
> Yes, and not just the standard formatters — IIUC, it should be all formatters!
> I've just now sent an email to lwgchair with subject line "Request new LWG issue: formatter<T>::format should be const-qualified".
> I've just now sent an email to lwgchair with subject line "Request new LWG issue: formatter<T>::format should be const-qualified".

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112017