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.

Sign up for GitHub

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

Jump to bottom

[LWG motion 8] P2845R8 Formatting of std::filesystem::path #6910

Merged
merged 1 commit into from Apr 16, 2024
Merged

[LWG motion 8] P2845R8 Formatting of std::filesystem::path #6910

merged 1 commit into from Apr 16, 2024

Conversation

burblebee
Copy link
Contributor

@burblebee burblebee commented Mar 28, 2024 •
edited by jensmaurer

Editorial changes:

  • Used oxford commas, and added commas to help the parsing of longer sentences.
  • Named the sections "formatter support" for consistency.
  • Used "fmtr" instead of "fmt" in section names to avoid confusion with existing section names.
  • Moved the grammar for path-format-spec into the "general" section.

Fixes #6879.
Fixes cplusplus/papers#1516

@burblebee burblebee marked this pull request as ready for review March 28, 2024 00:36
@jensmaurer jensmaurer added this to the post-2024-03 milestone Mar 28, 2024
@jwakely
Copy link
Member

jwakely commented Mar 28, 2024

Named the sections "formatter support" for consistency.

Consistency with what? No other std::formatter specializations have a section name like that, e.g. [time.format] is called "Formatting" and [stacktrace.format] is called "Formatting support".

The formatter specialization provides std::format support, not formatter support.

Could this be "Formatting support" like [stacktrace.format] ?

jwakely
jwakely reviewed Mar 28, 2024
View reviewed changes
source/iostreams.tex
If \tcode{charT} is \keyword{char},
\tcode{path::value_type} is \keyword{wchar_t}, and
the literal encoding is UTF-8,
then the escaped path is transcoded from the native encoding for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "the escaped path" is correct here, but that's what the paper says so I'll ask LWG.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an LWG issue, the paper is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll leave this with you if that's OK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now LWG 4070.

@burblebee
Copy link
Contributor Author

Named the sections "formatter support" for consistency.

Consistency with what? No other std::formatter specializations have a section name like that, e.g. [time.format] is called "Formatting" and [stacktrace.format] is called "Formatting support".

The formatter specialization provides std::format support, not formatter support.

Could this be "Formatting support" like [stacktrace.format] ?

I was being consistent with "hash support", but sure, "formatting support" works, will go with that, thanks!

@burblebee burblebee requested a review from jwakely April 1, 2024 23:27
jwakely
jwakely reviewed Apr 2, 2024
View reviewed changes
source/iostreams.tex Outdated
If \tcode{charT} and \tcode{path::value_type} are the same
then no transcoding is performed.
Otherwise, transcoding is
\impldef{transcoding of the escaped path when \tcode{charT} and \tcode{path::value_type} differ}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. When the character types differ, transcoding is required whether the path is escaped or not.

Suggested change
\impldef{transcoding of the escaped path when \tcode{charT} and \tcode{path::value_type} differ}.
\impldef{transcoding of a formatted path when \tcode{charT} and \tcode{path::value_type} differ}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could address this as part of the LWG I'm going to open, but I worry that the index text will be forgotten, because it's not visible in this subclause (only the words "implementation defined" are shown here, the rest is in the annex).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please open an issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't open issues against unmerged wording.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use your suggestion for now, and I'll send you a reminder when you've opened the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by LWG 4070 too.

@tkoeppe tkoeppe force-pushed the motions-2024-03-lwg-8 branch 2 times, most recently from 4e894ff to f17e363 Compare April 16, 2024 09:52
@burblebee @tkoeppe
P2845R8 Formatting of std::filesystem::path
8fa6e0d 
Editorial changes:
* Used Oxford commas, and added commas to help the parsing of longer sentences.
* Named the sections "formatter support" for consistency.
* Used "fmtr" instead of "fmt" in section names to avoid confusion
  with existing section names.
* Moved the grammar for path-format-spec to the front of the subclause,
  and add an introductory half-sentence.
@tkoeppe tkoeppe merged commit 1a48eeb into main Apr 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwakely jwakely jwakely left review comments

@tkoeppe tkoeppe tkoeppe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
post-2024-03
Development

Successfully merging this pull request may close these issues.

[2024-03 LWG Motion 8] Formatting of std::filesystem::path P2845 R8 Formatting of std::filesystem::path
4 participants
@burblebee @jwakely @tkoeppe @jensmaurer