Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jwakely opened this issue Oct 12, 2022 · 13 comments
Closed

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

jwakely opened this issue Oct 12, 2022 · 13 comments
Labels
B2 - improvement Bucket 2 as described by P0592: bug fixes, performance improvements, integration fixes for/between e C++23 Targeted at C++23 IS Ship vehicle: IS LEWG Library Evolution ready-for-library-evolution-meeting-review This paper needs to be discussed at a Library Evolution meeting size - small paper size estimate
Milestone

Comments

@jwakely
Copy link
Member

jwakely commented Oct 12, 2022

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

Is the proposed direction a desirable change?

@jwakely jwakely added the LEWG Library Evolution label Oct 12, 2022
@brycelelbach brycelelbach added C++23 Targeted at C++23 IS Ship vehicle: IS B2 - improvement Bucket 2 as described by P0592: bug fixes, performance improvements, integration fixes for/between e size - small paper size estimate ready-for-library-evolution-meeting-review This paper needs to be discussed at a Library Evolution meeting labels Oct 25, 2022
@brycelelbach brycelelbach added the scheduled-for-library-evolution This paper has been scheduled for one of the groups: LEWG, LEWG Incubator, or a Mailing List review label Nov 7, 2022
@brycelelbach
Copy link

brycelelbach commented Nov 10, 2022

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 brycelelbach added the needs-revision Paper needs changes before it can proceed label Nov 12, 2022
@brycelelbach
Copy link

@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
Copy link
Member

JeffGarland commented Nov 12, 2022

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
Copy link

@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
Copy link
Member Author

jwakely commented Nov 18, 2022

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

@brevzin
Copy link
Collaborator

brevzin commented Nov 18, 2022

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} (fmtlib/fmt#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: fmtlib/fmt#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
Copy link

(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
Copy link

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

@brycelelbach brycelelbach added ready-for-library-evolution-meeting-review This paper needs to be discussed at a Library Evolution meeting and removed needs-revision Paper needs changes before it can proceed ready-for-library-evolution-electronic-poll This paper needs to undergo a Library Evolution electronic poll labels Jan 24, 2023
@brevzin
Copy link
Collaborator

brevzin commented Jan 24, 2023

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

@vitaut
Copy link

vitaut commented Jan 24, 2023

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

@mordante
Copy link

(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
Copy link

I'm closing this as we now have a paper, P2733 #1426

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

@brycelelbach
Copy link

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

@jensmaurer jensmaurer added this to the 2023-telecon milestone Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B2 - improvement Bucket 2 as described by P0592: bug fixes, performance improvements, integration fixes for/between e C++23 Targeted at C++23 IS Ship vehicle: IS LEWG Library Evolution ready-for-library-evolution-meeting-review This paper needs to be discussed at a Library Evolution meeting size - small paper size estimate
Projects
Development

No branches or pull requests

7 participants