cplusplus / papers

ISO/IEC JTC1 SC22 WG21 paper scheduling and management
612 stars 19 forks source link

LWG3776 Avoid parsing format-spec if it is not present or empty #1314

Closed jwakely closed 1 year ago

jwakely commented 1 year ago

Could LEWG please review this issue https://cplusplus.github.io/LWG/issue3776

Is the proposed direction a desirable change?

brycelelbach commented 1 year ago

2022-11-07 10:00 to 11:30 UTC-10 Kona Library Evolution Meeting

LWG3776: Avoid parsing format-spec if it is not present or empty

2022-11-07 10:00 to 11:30 UTC-10 Kona Library Evolution Minutes

Champion: Victor Zverovich (in-person)

Chair: Fabio Fracassi & Billy Baker

Minute Taker: Steve Downey

POLL: Relax the requirements table 74 and 75 to make the optimization allowed by the issue resolution of LWG3776 a QoI issue with additional changes to the handle class removed

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
1 9 2 1 1

Attendance: 16 + 5

# of Authors: 0

Author Position: n/a

Outcome: consensus in favour

POLL: Adopt the amended proposed resolution of LWG3776 "Avoid parsing format-spec if it is not present or empty". Return the issue to LWG for C++23 (to be confirmed by electronic polling)

Strongly Favor Weakly Favor Neutral Weakly Against Strongly Against
2 6 1 2 1

Attendance: 16 + 5

# of Authors: 0

Author Position: n/a

Outcome: weak consensus in favour

amend the issue and return to LWG

Next Steps

Amend the issue resolution and return to LWG (electronic poll).

brycelelbach commented 1 year ago

@jwakely and @JeffGarland can we get the proposed resolution of the issue amended as per above before I take the Library Evolution electronic poll on it? Otherwise I think I'd need a paper - I want it to be clear what exactly we're polling.

JeffGarland commented 1 year ago

Yes, we can look at the wording. The proposed change seems fine although certainly less strong than the originators proposal. That said, I'm suspicious of the 'is not present' -- unless I'm forgetting this api it's always 'there' -- but can be empty -- suspect we can remove that phrase.

brycelelbach commented 1 year ago

@JeffGarland I just want to have the proposed resolution updated so that it's clear what we are polling. I'm going to ask Victor to write a short paper.

jwakely commented 1 year ago

I believe there is new information about this being a bad idea that breaks things. @brevzin were you present for the LEWG discussion?

brevzin commented 1 year ago

I wasn't at the discussion, but this is a bad idea that breaks things (or otherwise we can come up with a different design).

The current design for formatting ranges is that there is a set_debug_format() function that can be called. Notably, this is nullary, it's not set_debug_format(true) vs set_debug_format(false). That means you can't call it in formatter() if you're only conditionally doing debug formatting, you have to call it in parse() when you recognize that that the format-spec is a debug spec (which, for a range, includes an empty/absent format-spec).

I recently fixed a bug in {fmt} (https://github.com/fmtlib/fmt/issues/2634) whose cause was exactly this - failing to call parse() in the empty format-spec case, which meant that some formatters were not setting debug formatting (PR: https://github.com/fmtlib/fmt/pull/3158).

So if we want to do this (allow omitting calls to parse), then we need to change the debug formatting design to allow it to be disabled. But if we don't do the latter, we cannot do the former.

mordante commented 1 year ago

(I missed this GitHub issue before.) I filed this LWG issue, and I agree there is some interaction with formatting ranges and Improve default container formatting. While implementing these papers in libc++ I noticed that the tuple formatter requires parse to be called, but on the other hand it doesn't call parse on its own underlying formatter. So nesting tuples doesn't work as expected/intended:

std::format("{}", std::make_tuple('a')); //  ('a')
std::format("{}", std::make_tuple(std::make_tuple('a')); // ((a))

I have the strong suspicion there are more related issues, for example a formatter for a std::vector<char> is not a debug-enabled specialization. I am still investigating whether there are more issues in this regard. I expect to finish that early next week.

brycelelbach commented 1 year ago

We will revisit this at Issaquah. @brevzin will you be there?

brevzin commented 1 year ago

Yes, but I won't arrive 'til late Tuesday evening. I'll try to participate remotely Monday-Tuesday tho.

vitaut commented 1 year ago

There is a paper to fix this and related issues: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2733r0.html.

mordante commented 1 year ago

(I missed this GitHub issue before.) I filed this LWG issue, and I agree there is some interaction with formatting ranges and Improve default container formatting. While implementing these papers in libc++ I noticed that the tuple formatter requires parse to be called, but on the other hand it doesn't call parse on its own underlying formatter. So nesting tuples doesn't work as expected/intended:

std::format("{}", std::make_tuple('a')); //  ('a')
std::format("{}", std::make_tuple(std::make_tuple('a')); // ((a))

I have the strong suspicion there are more related issues, for example a formatter for a std::vector<char> is not a debug-enabled specialization. I am still investigating whether there are more issues in this regard. I expect to finish that early next week.

@vitaut's paper addresses these issues.

brycelelbach commented 1 year ago

I'm closing this as we now have a paper, P2733 https://github.com/cplusplus/papers/issues/1426

@brevzin and @vitaut can you both confirm that discussion of this can continue with P2733?

brycelelbach commented 1 year ago

@JeffGarland Can we close this Library issue and point to the paper?