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 inherent versions of MaybeUninit methods for slices #129259

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Aug 18, 2024

This is my attempt to un-stall #63569 and #79995, by creating methods that mirror the existing MaybeUninit API:

impl<T> MaybeUninit<T> {
    pub fn write(&mut self, value: T) -> &mut T;
    pub unsafe fn assume_init_drop(&mut self);
    pub unsafe fn assume_init_ref(&self) -> &T;
    pub unsafe fn assume_init_mut(&mut self) -> &mut T;
}

Offering these new methods:

impl<T> [MaybeUninit<T>] {
    pub fn write_copy_of_slice(&mut self, value: &[T]) -> &mut [T] where T: Copy;
    pub fn write_clone_of_slice(&mut self, value: &[T]) -> &mut [T] where T: Clone;
    pub unsafe fn assume_init_drop(&mut self);
    pub unsafe fn assume_init_ref(&self) -> &[T];
    pub unsafe fn assume_init_mut(&mut self) -> &mut [T];
}

Since the assume_init methods are identical to those on non-slices, they feel pretty natural. The main issue with the write methods is naming, as discussed in #79995 among other places. My rationale:

  • The term "write" should be in them somewhere, to mirror the other API, and this pretty much automatically makes them not collide with any other inherent slice methods.
  • I chose write_clone_of_slice and write_copy_of_slice since clone and copy are being used as nouns here, whereas they're being used as verbs in clone_from_slice and copy_from_slice.

I'm hoping these don't require an ACP since they're just refining upon existing APIs, but I can write one up if you prefer.

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

r? @tgross35

rustbot has assigned @tgross35.
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 Aug 18, 2024
@rust-log-analyzer

This comment has been minimized.

// guard is needed b/c panic might happen during a clone
let mut guard = Guard { slice: self, initialized: 0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

A test for panic during clone would probably be good to verify expected behavior

///
/// [`Vec<T>`]: ../../std/vec/struct.Vec.html
#[unstable(feature = "maybe_uninit_slice", issue = "63569")]
pub unsafe fn assume_init_drop(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The assume_init_* methods could probably use examples.

@tgross35
Copy link
Contributor

The implementation looks pretty reasonable to me, but as this is an API decision:

r? libs-api

Is the intent here to replace the existing copy_from_slice and clone_from_slice? If so, those should be removed.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 19, 2024
@rustbot rustbot assigned BurntSushi and unassigned tgross35 Aug 19, 2024
@clarfonthey
Copy link
Contributor Author

I decided to not delete them since so many people are using these methods, but I can deprecate them depending on what people think is best.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:20d3b4d4a2629cbf7865cdbf92fe47512a7c96658c24253a045ff38e8075cd7fb37ca6fcadfa6e6d093333943ad24f6fc4f163ec5b74fd940de9d5bb03eb4d3b:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.75s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
##[group]Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
    Finished `release` profile [optimized] target(s) in 0.13s
##[endgroup]
##[group]Testing stage0 Linkcheck (x86_64-unknown-linux-gnu)
core/primitive.slice.html:132: broken link - `/checkout/obj/build/x86_64-unknown-linux-gnu/std/vec/struct.Vec.html`
std/primitive.slice.html:276: broken link - `/checkout/obj/build/x86_64-unknown-linux-gnu/std/vec/struct.Vec.html`
number of HTML files scanned: 43652
number of HTML redirects found: 13723
number of links checked: 3237221
number of links ignored due to external: 78841
number of links ignored due to external: 78841
number of links ignored due to exceptions: 17
number of intra doc links ignored: 8
errors found: 2
found some broken links
Command has failed. Rerun with -v to see more details.
  local time: Sun Sep  1 20:45:56 UTC 2024
  network time: Sun, 01 Sep 2024 20:45:56 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 20, 2024
@Amanieu
Copy link
Member

Amanieu commented Nov 26, 2024

We discussed this in the libs-api meeting. We're happy with these changes, but we would also like to see similar changes for the fill (#117428) and as_bytes (#93092) methods so that all slice-related methods are turned into inherent methods. Note that fill will need to be renamed since it conflicts with the generic fill method on slices (perhaps fill_init?).

Also, the old methods on MaybeUninit should be removed (in a separate PR) so that we don't have duplicate methods that do the same thing.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 26, 2024
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. T-libs-api Relevant to the library API 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