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

Avoid panic branch in EscapeDefault::backslash and EscapeDebug::backslash. #121805

Closed

Conversation

reitermarkus
Copy link
Contributor

See japaric/ufmt#52 and https://github.com/andrewgazelka/ufmt/blob/e97ce1a86cfcc9fa36ffab6ee62762c00e4b3fc9/src/impls/core.rs#L62.

This avoids the unreachable panicking branch in char::escape_debug.

Not sure what the best way to test this is.

@rustbot
Copy link
Collaborator

rustbot commented Feb 29, 2024

r? @m-ou-se

rustbot has assigned @m-ou-se.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 29, 2024
@reitermarkus
Copy link
Contributor Author

r?

@rustbot
Copy link
Collaborator

rustbot commented Mar 25, 2024

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@joboet
Copy link
Member

joboet commented Apr 8, 2024

Are you sure that that's where the panic comes from? As far as I can tell, LLVM is able to optimize this out.

@joboet joboet assigned joboet and unassigned m-ou-se Apr 8, 2024
@reitermarkus
Copy link
Contributor Author

reitermarkus commented Apr 10, 2024

I assume the panic_never crate (used to test ufmt) only works before optimization, i.e. it won't compile (or more specifically link, I guess) if there is any panicking branch, even if it is eventually optimized out.

@joboet
Copy link
Member

joboet commented Apr 15, 2024

This is the wrong panic, the assert that cannot be optimized out is in ExactSizeIterator::len.

See how the BB in bjorn's Godbolt example is only reached from two comparisons that depend on the result of EscapeIterInner::size_hint? I think this is this assertion:

assert_eq!(upper, Some(lower));

It will never trigger but cannot be optimized out since EscapeIterInner::size_hint isn't marked #[inline]:

fn size_hint(&self) -> (usize, Option<usize>) {

I'll close this PR, but feel free to r? me on a PR that adds the #[inline] hint.

@reitermarkus
Copy link
Contributor Author

Thanks for looking into this, @joboet, I have opened a PR to add the inline attribute here: #124307

fmease added a commit to fmease/rust that referenced this pull request May 15, 2024
…t-inline, r=joboet

Optimize character escaping.

Allow optimization of panicking branch in `EscapeDebug`, see rust-lang#121805.

r? `@joboet`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2024
Rollup merge of rust-lang#124307 - reitermarkus:escape-debug-size-hint-inline, r=joboet

Optimize character escaping.

Allow optimization of panicking branch in `EscapeDebug`, see rust-lang#121805.

r? `@joboet`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants