Update char::escape_debug_ext to handle different escapes in strings and chars by osa1 · Pull Request #83079 · rust-lang/rust

osa1 · 2021-03-13T08:28:47Z

The program

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

would previously print

'\"'
"\'"

With this patch it now prints:

'"'
"'"

rust-highfive · 2021-03-13T08:28:50Z

r? @m-ou-se

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

osa1 · 2021-03-13T08:30:30Z

I'll update docs of escape_debug_ext and escape_debug.

library/core/src/char/methods.rs

osa1 · 2021-03-13T14:28:36Z

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

osa1 · 2021-03-13T14:54:27Z

library/core/src/char/methods.rs

Should I introduce a struct for these bool args?

osa1 · 2021-03-13T15:24:36Z

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

osa1 · 2021-03-14T08:51:06Z

library/core/src/fmt/mod.rs

Debug output of str now escapes " but not '.

osa1 · 2021-03-14T08:51:23Z

library/core/src/fmt/mod.rs

Debug output of char not escapes ' but not ".

osa1 · 2021-03-14T09:17:53Z

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 · 2021-03-14T09:18:18Z

library/core/src/char/methods.rs

Escapes both ' and ", as before.

osa1 · 2021-03-14T09:18:33Z

library/core/src/str/mod.rs

Escapes both ' and ", as before.

osa1 · 2021-03-14T09:18:44Z

library/core/src/str/mod.rs

Escapes both ' and ", as before.

osa1 · 2021-03-15T11:05:18Z

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

joshtriplett · 2021-03-16T08:49:13Z

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?

enum EscapeChars {
    EscapeSingleQuote,
    EscapeDoubleQuote,
    EscapeBothQuotes,
}

impl EscapeChars {
    fn escape_single_quote(&self) -> bool {
        match self {
            ...
        }
    }

    fn escape_double_quote(&self) -> bool {
        match self {
            ...
        }
    }
}

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

bors · 2021-03-19T18:25:14Z

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

osa1 · 2021-03-21T20:36:10Z

@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 · 2021-03-26T07:59:39Z

Ping @m-ou-se

m-ou-se · 2021-03-26T08:14:26Z

Thanks!

@bors r+

bors · 2021-03-26T08:14:28Z

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

…vs. chars Fixes #83046 The program fn main() { println!("{:?}", '"'); println!("{:?}", "'"); } would previously print '\"' "\'" With this patch it now prints: '"' "'"

osa1 · 2021-03-26T08:25:49Z

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

m-ou-se · 2021-03-26T08:33:59Z

@bors r+

bors · 2021-03-26T08:34:01Z

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

bors · 2021-03-26T09:11:23Z

⌛ Testing commit 819247f with merge 4137088...

bors · 2021-03-26T11:57:37Z

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

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.

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

The change in [1] is now in stable. [1]: rust-lang/rust#83079

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

rust-highfive assigned m-ou-se Mar 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021

osa1 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 chars Mar 13, 2021

osa1 marked this pull request as draft March 13, 2021 08:30

This comment has been minimized.

Sign in to view

m-ou-se reviewed Mar 13, 2021

View reviewed changes

library/core/src/char/methods.rs Outdated Show resolved Hide resolved

osa1 marked this pull request as ready for review March 13, 2021 14:16

osa1 requested a review from m-ou-se March 13, 2021 14:17

This comment has been minimized.

Sign in to view

osa1 commented Mar 13, 2021

View reviewed changes

library/core/src/char/methods.rs Outdated

Copy link

Contributor Author

osa1 Mar 13, 2021

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?

This comment has been minimized.

Sign in to view