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

impl<T> From<Option<&T>> for *const T #93664

Closed
wants to merge 1 commit into from

Conversation

kupiakos
Copy link
Contributor

@kupiakos kupiakos commented Feb 4, 2022

And same for *mut T. I wasn't sure whether it was worth defining the trait as const for the mut version, considering that's a separate feature for mut refs in const. I also considered having the implementation be the transmute this optimizes to, but decided against it.

This also puts off defining the impl for T: ?Sized since the validity of creating a null *const dyn Trait is not settled.

This is my first contribution to the repo - please let me know if I missed something obvious :)

Zulip thread

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2022
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Feb 6, 2022
@cuviper cuviper added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 9, 2022
@kupiakos
Copy link
Contributor Author

kupiakos commented Mar 9, 2022

Is there anything else I should be doing for this?

@bors
Copy link
Contributor

bors commented Apr 2, 2022

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

@Mark-Simulacrum
Copy link
Member

Hi! Apologies for the long delay. I've re-assigned this PR to @joshtriplett since it'll need an FCP to be merged (adding new trait impls).

@camelid
Copy link
Member

camelid commented Feb 11, 2023

r? rust-lang/libs-api

@rustbot rustbot assigned BurntSushi and unassigned joshtriplett Feb 11, 2023
@dtolnay
Copy link
Member

dtolnay commented Mar 14, 2023

Reassigning BurntSushi's reviews because based on git log -i --grep=r=burntsushi the last time they did rust-lang/rust reviews was over 3 years ago.

r? rust-lang/libs-api

@rustbot rustbot assigned m-ou-se and unassigned BurntSushi Mar 14, 2023
@pitaj
Copy link
Contributor

pitaj commented Apr 30, 2023

Ping from triage. @kupiakos FYI you still have merge conflicts. Please @rustbot review when you've resolved them

@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 Apr 30, 2023
@rust-log-analyzer

This comment has been minimized.

@kupiakos
Copy link
Contributor Author

Looks like the story around const traits has changed. In order to allow .into() in const, I would have to add #[const_trait] on both From and Into, which they don't do at the moment. That change seemed out-of-scope for this PR, and so I no longer have the From<&T> for *const T conversion be const-capable.

@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 Jun 28, 2023
@rust-log-analyzer

This comment has been minimized.

@kupiakos
Copy link
Contributor Author

kupiakos commented Jun 28, 2023

Ah, I sent for review too soon. It should be right now. The failing CI and subsequent fix actually shows this is technically a breaking change:

<*const _>::from(x) where x: &T is...weird, but did compile. That line causes the from to resolve to the blanket impl From<T> for T with T = *const Enum. Now, it's ambiguous and results in an error. There has never been a impl<T> From<&T> for *const T which that line implies exists. This being a stable breaking change might be enough reason to reject the PR entirely.

@Dylan-DPC
Copy link
Member

Closing this as it is a breaking change and the author agrees it is better to close it

@Dylan-DPC Dylan-DPC closed this Feb 3, 2024
@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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.