Skip to content

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