Skip to content

Use NonNull::new_unchecked and NonNull::len in rustc_arena. #110921

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 29, 2023
Merged

Use NonNull::new_unchecked and NonNull::len in rustc_arena. #110921

merged 1 commit into from
Apr 29, 2023

Conversation

nullptrderef29
Copy link
Contributor

@nullptrderef29 nullptrderef29 commented Apr 28, 2023 •
edited
Loading

This avoids a few extra dereferences as well as an unwrap.

According to the docs for NonNull::len this also ensures that:

This function is safe, even when the non-null raw slice cannot be dereferenced to a slice because the pointer does not have a valid address.

I am also fairly sure that the unwrap is unneeded in this case, as the docs for Box::into_raw also state:

Consumes the Box, returning a wrapped raw pointer.
The pointer will be properly aligned and non-null.

@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 28, 2023
Noratrieb
Noratrieb reviewed Apr 28, 2023
View reviewed changes
compiler/rustc_arena/src/lib.rs
@@ -74,7 +74,7 @@ impl<T> ArenaChunk<T> {
#[inline]
unsafe fn new(capacity: usize) -> ArenaChunk<T> {
ArenaChunk {
storage: NonNull::new(Box::into_raw(Box::new_uninit_slice(capacity))).unwrap(),
storage: NonNull::new_unchecked(Box::into_raw(Box::new_uninit_slice(capacity))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice example of why unsafe_op_in_unsafe_fn not being on by default is a big mistake. The safety condition of the function (which I actually don't know because it's not documented :/) has nothing to do with the safety condition of new_umchecked.

I'd love to take a PR denying that lint on rustc_arena if you want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have created a PR for this here #110950.

@Noratrieb
Copy link
Member

r? Nilstrieb
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 28, 2023

📌 Commit 618841b has been approved by Nilstrieb

It is now in the queue for this repository.

@rustbot rustbot assigned Noratrieb and unassigned b-naber Apr 28, 2023
@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 Apr 28, 2023
bugadani
bugadani reviewed Apr 28, 2023
View reviewed changes
compiler/rustc_arena/src/lib.rs
@@ -74,7 +74,7 @@ impl<T> ArenaChunk<T> {
#[inline]
unsafe fn new(capacity: usize) -> ArenaChunk<T> {
ArenaChunk {
storage: NonNull::new(Box::into_raw(Box::new_uninit_slice(capacity))).unwrap(),
storage: NonNull::new_unchecked(Box::into_raw(Box::new_uninit_slice(capacity))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
storage: NonNull::new_unchecked(Box::into_raw(Box::new_uninit_slice(capacity))),
storage: NonNull::from(Box::leak(Box::new_uninit_slice(capacity))),

@GuillaumeGomez GuillaumeGomez mentioned this pull request Apr 28, 2023
Closed
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 28, 2023
@GuillaumeGomez
Rollup merge of rust-lang#110921 - JohnBobbo96:rustc_arena_nonnull, r…
3962bfd 
…=Nilstrieb

Use `NonNull::new_unchecked` and `NonNull::len` in `rustc_arena`.

This avoids a few extra dereferences as well as an `unwrap`.

According to the docs for [`NonNull::len`](https://doc.rust-lang.org/std/ptr/struct.NonNull.html#method.len) this also ensures that:

> This function is safe, even when the non-null raw slice cannot be dereferenced to a slice because the pointer does not have a valid address.

I am also fairly sure that the `unwrap` is unneeded in this case, as the docs for [`Box::into_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw) also state:

> Consumes the Box, returning a wrapped raw pointer.
**The pointer will be properly aligned and non-null.**
@bors
Copy link
Collaborator

bors commented Apr 28, 2023

⌛ Testing commit 618841b with merge d1500bb15183239bf2d3423529fe31e86ada738d...

@bors
Copy link
Collaborator

bors commented Apr 28, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 28, 2023
@Noratrieb
Copy link
Member

It failed to install awscli, seems spurious.
@bors retry

@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 Apr 28, 2023
@bors
Copy link
Collaborator

bors commented Apr 28, 2023

⌛ Testing commit 618841b with merge 332e1cbb35ef7e9287e50b458751d98390f9612c...

@bors
Copy link
Collaborator

bors commented Apr 28, 2023

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 28, 2023
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2023
@Noratrieb
Copy link
Member

@bors treeclosed=50

@ehuss
Copy link
Contributor

ehuss commented Apr 28, 2023

@bors retry

python setuptools issue

@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 Apr 28, 2023
@bors bors mentioned this pull request Apr 28, 2023
Merged
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

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

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

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

This was referenced Apr 28, 2023
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 28, 2023
@matthiaskrgr
Rollup merge of rust-lang#110921 - JohnBobbo96:rustc_arena_nonnull, r…
4d32087 
…=Nilstrieb

Use `NonNull::new_unchecked` and `NonNull::len` in `rustc_arena`.

This avoids a few extra dereferences as well as an `unwrap`.

According to the docs for [`NonNull::len`](https://doc.rust-lang.org/std/ptr/struct.NonNull.html#method.len) this also ensures that:

> This function is safe, even when the non-null raw slice cannot be dereferenced to a slice because the pointer does not have a valid address.

I am also fairly sure that the `unwrap` is unneeded in this case, as the docs for [`Box::into_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw) also state:

> Consumes the Box, returning a wrapped raw pointer.
**The pointer will be properly aligned and non-null.**
This was referenced Apr 28, 2023
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2023
@bors
Auto merge of rust-lang#110967 - matthiaskrgr:rollup-vfbl7gm, r=matth…
7a96158 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#110877 (Provide better type hints when a type doesn't support a binary operator)
 - rust-lang#110917 (only error combining +whole-archive and +bundle for rlibs)
 - rust-lang#110921 (Use `NonNull::new_unchecked` and `NonNull::len` in `rustc_arena`.)
 - rust-lang#110927 (Encoder/decoder cleanups)
 - rust-lang#110944 (share BinOp::Offset between CTFE and Miri)
 - rust-lang#110948 (run-make test: using single quotes to not trigger the shell)
 - rust-lang#110957 (Fix an ICE in conflict error diagnostics)
 - rust-lang#110960 (fix false negative for `unused_mut`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4751c85 into rust-lang:master Apr 29, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 29, 2023
@nullptrderef29 nullptrderef29 deleted the rustc_arena_nonnull branch April 29, 2023 21:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bugadani bugadani bugadani left review comments

@Noratrieb Noratrieb Noratrieb approved these changes

Assignees

@Noratrieb Noratrieb

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

Successfully merging this pull request may close these issues.
8 participants
@nullptrderef29 @rustbot @Noratrieb @bors @ehuss @rust-log-analyzer @bugadani @b-naber