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.

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

[core/fmt] Replace checked slice indexing by unchecked to support panic-free code #132473

Merged
merged 3 commits into from
Nov 5, 2024
Merged

[core/fmt] Replace checked slice indexing by unchecked to support panic-free code #132473

merged 3 commits into from
Nov 5, 2024

Conversation

ZhekaS
Copy link

@ZhekaS ZhekaS commented Nov 1, 2024

Fixes #126425

Replace the potentially panicking [] indexing with get_unchecked() to prevent linking with panic-related code.

@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joboet (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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 Nov 1, 2024
@zachs18
Copy link
Contributor

zachs18 commented Nov 3, 2024

The safety comment could also mention that curr being decremented cannot overflow to cause it to be out of bounds.

the reasoning

curr is initialized to buf.len(), and is only ever decremented, [snip...]

buf has length 128, and curr is decremented exactly once for each digit, so for curr to overflow would require that the number to be formatted have more that 128 digits in the particular base (2, 8, or 16), which is currently not possible since u128 is the largest fixed-width integer type, and usize::BITS <= 64 on all currently supported platforms.

I suppose a const _: () = assert!(T::BITS <= 128) or similar could be added to this code to ensure that any future platform with usize::BITS > 128 would fail to compile unless this code was updated to account for it, (but I imagine there are several other places in the standard library which makes a similar assumption).

from here: #126425 (comment)

@joboet
Copy link
Member

joboet commented Nov 3, 2024

That sounds good, please add such an explanation to the comment.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2024
@ZhekaS
Copy link
Author

ZhekaS commented Nov 4, 2024

That sounds good, please add such an explanation to the comment. @rustbot author

Addressed
@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2024
Eugene Shamis added 3 commits November 4, 2024 12:36
Replace checked slice indexing by unchecked to support panic-free code
02a1ab8 
Fixes rust-lang#126425

Replace the potentially panicking `[]` indexing with `get_unchecked()`
to prevent linking with panic-related code.
Updated SAFETY comment to address underflow
37f48da
Fixed typo, rebased
65d8f1b
@joboet
Copy link
Member

joboet commented Nov 4, 2024

Thank you!
@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2024

📌 Commit 65d8f1b has been approved by joboet

It is now in the queue for this repository.

@bors bors 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 Nov 4, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 4, 2024
@workingjubilee
Rollup merge of rust-lang#132473 - ZhekaS:core_fmt_radix_no_panic, r=…
9f1dcd5 
…joboet

[core/fmt] Replace checked slice indexing by unchecked to support panic-free code

Fixes rust-lang#126425

Replace the potentially panicking `[]` indexing with `get_unchecked()` to prevent linking with panic-related code.
@workingjubilee workingjubilee mentioned this pull request Nov 4, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2024
@bors
Auto merge of rust-lang#132616 - workingjubilee:rollup-0ykzbyb, r=wor…
404c53a 
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#129884 (mark some target features as 'forbidden' so they cannot be (un)set with -Ctarget-feature)
 - rust-lang#132153 (Stabilise `const_char_encode_utf16`.)
 - rust-lang#132473 ([core/fmt] Replace checked slice indexing by unchecked to support panic-free code)
 - rust-lang#132571 (add const_eval_select macro to reduce redundancy)
 - rust-lang#132587 (Revert "Avoid nested replacement ranges" from rust-lang#129346.)
 - rust-lang#132596 ([rustdoc] Fix `--show-coverage` when JSON output format is used)
 - rust-lang#132598 (Clippy: Move some attribute lints to be early pass (post expansion))
 - rust-lang#132601 (Update books)
 - rust-lang#132606 (Improve example of `impl Pattern for &[char]`)
 - rust-lang#132609 (docs: fix grammar in doc comment at unix/process.rs)

r? `@ghost`
`@rustbot` modify labels: rollup
tgross35 added a commit to tgross35/rust that referenced this pull request Nov 5, 2024
@tgross35
Rollup merge of rust-lang#132473 - ZhekaS:core_fmt_radix_no_panic, r=…
d35f5d5 
…joboet

[core/fmt] Replace checked slice indexing by unchecked to support panic-free code

Fixes rust-lang#126425

Replace the potentially panicking `[]` indexing with `get_unchecked()` to prevent linking with panic-related code.
@tgross35 tgross35 mentioned this pull request Nov 5, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2024
@bors
Auto merge of rust-lang#132621 - tgross35:rollup-q2zhwk3, r=tgross35
05c9da1 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#131153 (Improve duplicate derive Copy/Clone diagnostics)
 - rust-lang#131341 (Support clobber_abi and vector registers (clobber-only) in PowerPC inline assembly)
 - rust-lang#132025 (fix suggestion for diagnostic error E0027)
 - rust-lang#132153 (Stabilise `const_char_encode_utf16`.)
 - rust-lang#132303 (More tests for non-exhaustive C-like enums in FFI)
 - rust-lang#132473 ([core/fmt] Replace checked slice indexing by unchecked to support panic-free code)
 - rust-lang#132598 (Clippy: Move some attribute lints to be early pass (post expansion))
 - rust-lang#132606 (Improve example of `impl Pattern for &[char]`)
 - rust-lang#132609 (docs: fix grammar in doc comment at unix/process.rs)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee workingjubilee mentioned this pull request Nov 5, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2024
@bors
Auto merge of rust-lang#132631 - workingjubilee:rollup-dx0krqp, r=wor…
977ad50 
…kingjubilee

Rollup of 4 pull requests

Successful merges:

 - rust-lang#132153 (Stabilise `const_char_encode_utf16`.)
 - rust-lang#132473 ([core/fmt] Replace checked slice indexing by unchecked to support panic-free code)
 - rust-lang#132600 (PassWrapper: adapt for new parameter in LLVM)
 - rust-lang#132630 (triagebot: ping wg-const-eval when relevant files change)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2024
@bors
Auto merge of rust-lang#132631 - workingjubilee:rollup-dx0krqp, r=wor…
bc5cf99 
…kingjubilee

Rollup of 4 pull requests

Successful merges:

 - rust-lang#132153 (Stabilise `const_char_encode_utf16`.)
 - rust-lang#132473 ([core/fmt] Replace checked slice indexing by unchecked to support panic-free code)
 - rust-lang#132600 (PassWrapper: adapt for new parameter in LLVM)
 - rust-lang#132630 (triagebot: ping wg-const-eval when relevant files change)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 57f64c6 into rust-lang:master Nov 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2024
@rust-timer
Unrolled build for rust-lang#132473
ffad505 
Rollup merge of rust-lang#132473 - ZhekaS:core_fmt_radix_no_panic, r=joboet

[core/fmt] Replace checked slice indexing by unchecked to support panic-free code

Fixes rust-lang#126425

Replace the potentially panicking `[]` indexing with `get_unchecked()` to prevent linking with panic-related code.
@ZhekaS ZhekaS deleted the core_fmt_radix_no_panic branch November 5, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joboet joboet

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.

UpperHex formatting might panic
5 participants
@ZhekaS @rustbot @zachs18 @joboet @bors