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

Add as_(mut_)ptr and as_(mut_)slice to raw array pointers #119411

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

yotamofek
Copy link
Contributor

@yotamofek yotamofek commented Dec 29, 2023

Hey, first time contributing to the standard libraries so not completely sure about the process.

These functions are complementary to the ones being added in #74265 . I found them missing on array pointers.

See also:

@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2023

r? @joshtriplett

(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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 29, 2023
@Noratrieb
Copy link
Member

Thank you for the PR! This PR contains a public library API change, which should follow the proper process: https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html

@yotamofek
Copy link
Contributor Author

@Nilstrieb thanks for the link!
Just making sure I'm parsing it correctly: it seems to me that if a proposed API has such a simple implementation, such as this one, then there's no need for an ACP?
Or is it better to go down the formal ACP route anyways?

@yotamofek yotamofek changed the title Add as_{mut}_ptr and as_{mut} slice to raw array pointers Add as_{mut}_ptr and as_{mut}_slice to raw array pointers Dec 29, 2023
@Noratrieb
Copy link
Member

I recommend opening an ACP anyways.

@yotamofek yotamofek changed the title Add as_{mut}_ptr and as_{mut}_slice to raw array pointers Add as_(mut_)ptr and as_(mut_)slice to raw array pointers Jan 10, 2024
@bors
Copy link
Contributor

bors commented Jan 20, 2024

☔ The latest upstream changes (presumably #120157) made this pull request unmergeable. Please resolve the merge conflicts.

@yotamofek
Copy link
Contributor Author

@joshtriplett just a friendly ping, in case you missed this one (which makes sense, I opened it on December 29th haha). Thank you!

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Sorry for the late response. I found some issues which need to be addressed before we can merge this (you don't have to address the "unimportant nit" ones if you don't want to).
Use @rustbot readywhen you've addressed the feedback.

Comment on lines +1782 to +1813
/// let arr: *const [i8; 3] = ptr::null();
/// assert_eq!(arr.as_ptr(), ptr::null());
Copy link
Member

Choose a reason for hiding this comment

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

unimportant nit: The example could be a bit more useful, showing a pointer to an actual array and then maybe passing as_ptr to copy_nonoverlapping to copy out of the array or something like that.

Comment on lines +2209 to +2224
/// let arr: *mut [i8; 3] = ptr::null_mut();
/// assert_eq!(arr.as_mut_ptr(), ptr::null_mut());
Copy link
Member

Choose a reason for hiding this comment

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

unimportant nit: same here

library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
@Noratrieb Noratrieb assigned Noratrieb and unassigned joshtriplett Jan 27, 2024
@Noratrieb Noratrieb 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 Jan 27, 2024
@yotamofek yotamofek force-pushed the array-ptr-get branch 2 times, most recently from dd8b7ee to 8836254 Compare January 27, 2024 15:39
@yotamofek
Copy link
Contributor Author

yotamofek commented Jan 27, 2024

@Nilstrieb thank you so much for your patient review, and terribly sorry for the silly copy-paste errors that I should have caught earlier. This is my first (somewhat) substantial contribution to rust, so I really appreciate it!

I elected to not change the examples for now. Also, made the mut methods const, because why not?

If github doesn't GC it, you can see just my CR fixes here: dd8b7ee

@rustbot ready

@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 Jan 27, 2024
library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
@Noratrieb Noratrieb 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 Feb 8, 2024
@bors
Copy link
Contributor

bors commented Feb 9, 2024

☔ The latest upstream changes (presumably #120676) made this pull request unmergeable. Please resolve the merge conflicts.

@yotamofek yotamofek force-pushed the array-ptr-get branch 2 times, most recently from 80b5af7 to 8775a7b Compare March 15, 2024 19:06
@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 Mar 16, 2024
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good now! I've left a response to your question, but if you can't be bothered to fix it that's okay, just say you're fine with it as-is (because I'm too, I just don't find it quite optimal) and then I'll approve it as-is. If you do want to fix it, fix it and I'll approve it.

library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
@yotamofek
Copy link
Contributor Author

Thanks for the review! 🙏 Pushed a fix.

@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2024

📌 Commit d0c0887 has been approved by Nilstrieb

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 Mar 16, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 17, 2024
Add as_(mut_)ptr and as_(mut_)slice to raw array pointers

Hey, first time contributing to the standard libraries so not completely sure about the process.

These functions are complementary to the ones being added in rust-lang#74265 . I found them missing on array pointers.

See also:
- ACP: rust-lang/libs-team#321
- Tracking issue: rust-lang#119834
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#119411 (Add as_(mut_)ptr and as_(mut_)slice to raw array pointers)
 - rust-lang#122248 (Respect stage0 sysroot when compiling rmake.rs with COMPILETEST_FORCE_STAGE0)
 - rust-lang#122295 (mir-opt: always run tests for the current target)
 - rust-lang#122574 (Register LLVM handlers for bad-alloc / OOM)
 - rust-lang#122608 (Move check-cfg diagnostic logic into a separate file)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3c07321 into rust-lang:master Mar 17, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
Rollup merge of rust-lang#119411 - yotamofek:array-ptr-get, r=Nilstrieb

Add as_(mut_)ptr and as_(mut_)slice to raw array pointers

Hey, first time contributing to the standard libraries so not completely sure about the process.

These functions are complementary to the ones being added in rust-lang#74265 . I found them missing on array pointers.

See also:
- ACP: rust-lang/libs-team#321
- Tracking issue: rust-lang#119834
@yotamofek yotamofek deleted the array-ptr-get branch March 17, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants