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

Fri Nov 12 08:18:10 PST 2021

Mordante 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()) {
----------------
vitaut wrote:
> 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!
> 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.

Yes it should indeed be a separate patch when we want to make this change. Currently the format function can't be const. It updates the requested width of the format string when it's supplied as argument to the format function.

================
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()) {
----------------
Mordante wrote:
> vitaut wrote:
> > 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!
> > 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.
> 
> Yes it should indeed be a separate patch when we want to make this change. Currently the format function can't be const. It updates the requested width of the format string when it's supplied as argument to the format function.
> 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.

Just curious will it be required that format strings are compiled in the future? AFAIK P2216 only requires compile-time validation but not compilation. (Obviously it would be a good idea to avoid parsing the format string again at runtime.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112017