Get rid of `EscapeDebugInner`. #138237

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

Open

reitermarkus wants to merge 1 commit into rust-lang:master from reitermarkus:remove-escape-debug-inner

Contributor

reitermarkus commented Mar 8, 2025

I read the note on EscapeDebugInner and thought I'd give it a try.

rustbot assigned thomcc

Collaborator

rustbot commented Mar 8, 2025

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review T-libs labels

reitermarkus force-pushed the remove-escape-debug-inner branch 3 times, most recently from 57c0a80 to 0854482 Compare

March 8, 2025 20:52

hkBst reviewed

View reviewed changes

library/core/src/char/mod.rs Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

hkBst reviewed

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

reitermarkus commented

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

reitermarkus commented

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

reitermarkus commented

View reviewed changes

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

reitermarkus requested a review from hkBst

March 10, 2025 17:43

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

Member

hkBst commented Mar 11, 2025 •

edited

Loading

-        // SAFETY: `self.data.escape_seq` contains an escape sequence, as is 
-        // guaranteed by the caller, and in that case `self.alive` is guaranteed 
+        // SAFETY: `self.data.escape_seq` contains an escape sequence, as is
+        // guaranteed by the caller, and in that case `self.alive` is guaranteed

I'm sure you know this, but if you highlight these lines, you can see that the trailing space is the problem.

tgross35 approved these changes

View reviewed changes

Contributor

tgross35 left a comment

Okay, this looks great to me! I left two comments requesting debug assertions to sanity check the preconditions, r=me after that (or if not possible) and a squash.

library/core/src/escape.rs Outdated Show resolved Hide resolved

library/core/src/escape.rs Outdated Show resolved Hide resolved

Contributor

tgross35 commented Jun 11, 2025

@rustbot author for the debug asserts + squash

rustbot added S-waiting-on-author and removed S-waiting-on-review labels

reitermarkus force-pushed the remove-escape-debug-inner branch 3 times, most recently from 9645c1a to 0d2d520 Compare

June 15, 2025 01:22

Contributor Author

reitermarkus commented Jun 15, 2025

rustbot added S-waiting-on-review and removed S-waiting-on-author labels

This comment has been minimized.

Sign in to view

reitermarkus force-pushed the remove-escape-debug-inner branch from 0d2d520 to 57d9fd7 Compare

June 15, 2025 19:37

This comment has been minimized.

Sign in to view


          Get rid of EscapeDebugInner.

ce457e1

reitermarkus force-pushed the remove-escape-debug-inner branch from 57d9fd7 to ce457e1 Compare

June 15, 2025 20:08

Contributor

tgross35 commented Jun 15, 2025

Thank you!

Collaborator

bors commented Jun 15, 2025

📌 Commit ce457e1 has been approved by tgross35

It is now in the queue for this repository.

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Contributor

tgross35 commented Jun 15, 2025

@bors rollup=maybe didn't seem to have any perf impact

tgross35 added a commit to tgross35/rust that referenced this pull request


          Rollup merge of rust-lang#138237 - reitermarkus:remove-escape-debug-i…

487a265

…nner, r=tgross35

Get rid of `EscapeDebugInner`.

I read the note on `EscapeDebugInner` and thought I'd give it a try.

tgross35 mentioned this pull request

Rollup of 7 pull requests #142558

Closed

bors added a commit that referenced this pull request


          Auto merge of #142558 - tgross35:rollup-ld8nn4w, r=tgross35

befb020

Rollup of 7 pull requests

Successful merges:

 - #138237 (Get rid of `EscapeDebugInner`.)
 - #140809 (Reduce special casing for the panic runtime)
 - #141990 (Implement send_signal for unix child processes)
 - #142082 (Refactor `rustc_attr_data_structures` documentation)
 - #142125 (Stabilize "file_lock" feature)
 - #142528 (clarify `rustc_do_not_const_check` comment)
 - #142530 (use `if let` guards where possible)

r? `@ghost`
`@rustbot` modify labels: rollup

bors added a commit that referenced this pull request


          Auto merge of #138237 - reitermarkus:remove-escape-debug-inner, r=tgr…

b0eab47

…oss35

Get rid of `EscapeDebugInner`.

I read the note on `EscapeDebugInner` and thought I'd give it a try.

Collaborator

bors commented Jun 16, 2025

⌛ Testing commit ce457e1 with merge b0eab47...

bors mentioned this pull request

Remove an njn: comment accidentaly left behind. #142561

Merged

Collaborator

rust-log-analyzer commented Jun 16, 2025

The job dist-i586-gnu-i586-i686-musl failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Collaborator

bors commented Jun 16, 2025

💔 Test failed - checks-actions

bors added S-waiting-on-review and removed S-waiting-on-bors labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review T-libs