Skip to content

Conversation

@Alphare
Copy link

@Alphare Alphare commented Nov 20, 2024

This is my first contribution because I basically stumbled upon this not being implemented and figured it would be a quick fix, sorry if I'm missing anything. I see that all other impls in this file have feature annotations, I'm not sure if this would need one.

@rustbot
Copy link
Collaborator

rustbot commented Nov 20, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jhpratt (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 20, 2024
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
#16 exporting to docker image format
#16 sending tarball 29.0s done
#16 DONE 31.7s
##[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-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
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-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--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-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
   Compiling compiler_builtins v0.1.138
error: implementation has missing stability attribute
   --> core/src/num/nonzero.rs:290:1
    |
290 | / impl<T> From<&NonZero<T>> for &T
291 | | where
292 | |     T: ZeroablePrimitive,
293 | | {
299 | |     }
300 | | }
    | |_^

@jhpratt
Copy link
Member

jhpratt commented Nov 20, 2024

Trait implementations require a stability attribute, which is why CI is failing. Unfortunately, they also cannot be unstable. For that reason, I'll have the team take a look at it as it will require FCP.

@rustbot label +I-libs-api-nominated

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

Seems reasonable to me.

I wonder if (in a separate PR) we should also add impl TryFrom<&T> for &NonZero<T>.

@Amanieu
Copy link
Member

Amanieu commented Nov 26, 2024

We discussed this in the libs-api meeting: the team feels that it would be better to provide this using the AsRef trait instead of From.

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

jhpratt commented Nov 28, 2024

Closing for that reason.

@jhpratt jhpratt closed this Nov 28, 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants