Skip to content

Update char::escape_debug_ext to handle different escapes in strings and chars - #83079

Merged
bors merged 1 commit into
rust-lang:masterfrom
osa1:issue83046
Mar 26, 2021
Merged

Update char::escape_debug_ext to handle different escapes in strings and chars#83079
bors merged 1 commit into
rust-lang:masterfrom
osa1:issue83046

Conversation

@osa1

@osa1osa1 commented Mar 13, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#83046

The program

fn main() {
println!("{:?}", '"');
println!("{:?}", "'");
}

would previously print

'\"'
"\'"

With this patch it now prints:

'"'
"'"

@rust-highfive

rust-highfive commented Mar 13, 2021

Copy link
Copy Markdown
Contributor

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021
@osa1osa1 changed the title Update char::escape_debug_ext to handle different escapes in strings …Update char::escape_debug_ext to handle different escapes in strings and charsMar 13, 2021
@osa1

osa1 commented Mar 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I'll update docs of escape_debug_ext and escape_debug.

@osa1
osa1 marked this pull request as draft March 13, 2021 08:30
@rust-log-analyzer

This comment has been minimized.

Sign in to view
m-ou-se
m-ou-se reviewed Mar 13, 2021
View reviewed changes
Comment threadlibrary/core/src/char/methods.rs Outdated
@osa1
osa1 marked this pull request as ready for review March 13, 2021 14:16
@osa1
osa1 requested a review from m-ou-seMarch 13, 2021 14:17
@osa1

osa1 commented Mar 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I'll add a test, which directory should I put it?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
osa1
osa1 commented Mar 13, 2021
View reviewed changes
Comment threadlibrary/core/src/char/methods.rs Outdated

@osa1osa1Mar 13, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Should I introduce a struct for these bool args?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@osa1

osa1 commented Mar 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I don't have time to tweak this more today, I'll get back to this later.

osa1
osa1 commented Mar 14, 2021
View reviewed changes
Comment threadlibrary/core/src/fmt/mod.rs Outdated

@osa1osa1Mar 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Debug output of str now escapes " but not '.

osa1
osa1 commented Mar 14, 2021
View reviewed changes
Comment threadlibrary/core/src/fmt/mod.rs Outdated

@osa1osa1Mar 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Debug output of char not escapes ' but not ".

@osa1

osa1 commented Mar 14, 2021

Copy link
Copy Markdown
ContributorAuthor

I believe this should currently be completely backwards compatible, other than Debug outputs for str and char.

For some reason x.py tests skips the tests and I couldn't find a way to force it to run, so I'll rely on CI to re-run the previously-failing tests.

osa1
osa1 commented Mar 14, 2021
View reviewed changes
Comment threadlibrary/core/src/char/methods.rs Outdated

@osa1osa1Mar 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Escapes both ' and ", as before.

osa1
osa1 commented Mar 14, 2021
View reviewed changes
Comment threadlibrary/core/src/str/mod.rs Outdated

@osa1osa1Mar 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Escapes both ' and ", as before.

osa1
osa1 commented Mar 14, 2021
View reviewed changes
Comment threadlibrary/core/src/str/mod.rs Outdated

@osa1osa1Mar 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Escapes both ' and ", as before.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@osa1

osa1 commented Mar 15, 2021

Copy link
Copy Markdown
ContributorAuthor

@m-ou-se This is ready for reviews now. I'll squash the commits once all is done.

@joshtriplett

joshtriplett commented Mar 16, 2021

Copy link
Copy Markdown
Member

This seems like a great idea. I think escape_debug_ext having three bool args right next to each other is error-prone, though. Would you consider adding an enum instead?

enumEscapeChars{EscapeSingleQuote,EscapeDoubleQuote,EscapeBothQuotes,}implEscapeChars{fnescape_single_quote(&self) -> bool{matchself{
...}}fnescape_double_quote(&self) -> bool{matchself{
...}}}

I'm hoping that'll produce generated code that's just as efficient.

@bors

bors commented Mar 19, 2021

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #83301) made this pull request unmergeable. Please resolve the merge conflicts.

@osa1

osa1 commented Mar 21, 2021

Copy link
Copy Markdown
ContributorAuthor

@joshtriplett I didn't quite understand how that enum would work, but I added a struct for to give names to the bool args.

I'll squash the commits before merge.

Ping @m-ou-se

@osa1

osa1 commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

Ping @m-ou-se

@m-ou-se

m-ou-se commented Mar 26, 2021

Copy link
Copy Markdown
Member

Thanks!

@bors r+

@bors

bors commented Mar 26, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 52db9708dbb5fbfde615fa2c64c92656b6e610ac has been approved by m-ou-se

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2021
@osa1
Update char::escape_debug_ext to handle different escapes in strings …
819247f
…vs. chars
Fixes#83046
The program
fn main() {
println!("{:?}", '"');
println!("{:?}", "'");
}
would previously print
'\"'
"\'"
With this patch it now prints:
'"'
"'"
@osa1

osa1 commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@m-ou-se Thanks for the review. I squashed the commits, could you approve again please?

@m-ou-se

m-ou-se commented Mar 26, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Mar 26, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 819247f has been approved by m-ou-se

@bors

bors commented Mar 26, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 819247f with merge 4137088...

@bors

bors commented Mar 26, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 4137088 to master...

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Mar 26, 2021
@bors
bors merged commit 4137088 into rust-lang:masterMar 26, 2021
@rustbotrustbot added this to the 1.53.0 milestone Mar 26, 2021
@osa1
osa1 deleted the issue83046 branch March 26, 2021 12:00
vickenty added a commit to vickenty/lang-c that referenced this pull request Apr 8, 2021
@vickenty
Don't depend on Debug impls in tests
f9e1f5d
Free lunch is over, Debug representation for str has changed
(rust-lang/rust#83079). Roll our own version based on the stable
`escape_debug()` method instead.
Also spell out all the enums while at it.
sharksforarms pushed a commit to sharksforarms/deku that referenced this pull request Apr 13, 2021
@abungay
fix: CI failure for Codecov/tarpaulin (#200)
33ce4f7
Run tarpaulin for code coverage on stable rust instead of nightly.
Nightly rust has our test cases failing due to the changed debug
printing of strings with single quotes "'" so our test cases using
should_panic(expected = <error message>) are failing.
See rust-lang/rust#83079
@WesleyRosenblumWesleyRosenblum mentioned this pull request May 6, 2021
Closed
@sharksforarmssharksforarms mentioned this pull request May 11, 2021
Merged
utkarshkukreti added a commit to utkarshkukreti/wn.rs that referenced this pull request Jul 10, 2021
@utkarshkukreti
update tests to work on latest stable
48fb49c
The change in [1] is now in stable.
[1]: rust-lang/rust#83079
@dtolnaydtolnay mentioned this pull request Apr 3, 2022
Merged
@tamirdtamird mentioned this pull request Aug 7, 2023
Closed
@coderabbitaicoderabbitaiBot mentioned this pull request Aug 10, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fmeasefmeasefmease left review comments
@m-ou-sem-ou-seAwaiting requested review from m-ou-se

Assignees

@m-ou-sem-ou-se

Labels

merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.53.0

Development

Successfully merging this pull request may close these issues.

StringDebug implementation prints escaped single quotes

8 participants

@osa1@rust-highfive@rust-log-analyzer@joshtriplett@bors@m-ou-se@fmease@rustbot