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.

Sign up for GitHub

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

Jump to bottom

const fn for option copied, take & replace #86828

Merged
merged 1 commit into from
Oct 4, 2021
Merged

const fn for option copied, take & replace #86828

merged 1 commit into from
Oct 4, 2021

Conversation

lambinoo
Copy link
Contributor

@lambinoo lambinoo commented Jul 2, 2021

Tracking issue: #67441

Adding const fn for the copied, take and replace method of Option. Also adding necessary unit test.

It's my first contribution so I am pretty sure I don't know what I'm doing but there's a first for everything!

@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 @joshtriplett (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 Jul 2, 2021
@jonas-schievink
Copy link
Contributor

I don't think Option::copied works, because const fn cannot have any trait bounds like T: Copy in scope

@lambinoo
Copy link
Contributor Author

lambinoo commented Jul 2, 2021 •
edited
Loading

That's where my knowledge kinda falls short.
But I've assumed that, since the entire impl block where Option::copied is defined is bounded by T: Copy, it should work even with a const fn. Is there something I am missing in this?

@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 2, 2021

Trait bound for const fn is gated under #![feature(const_fn_trait_bound)].

@jonas-schievink
Copy link
Contributor

#86808 also proposes using that feature in libstd, which is still pending decision by the libs team

@lambinoo
Copy link
Contributor Author

lambinoo commented Jul 2, 2021 •
edited
Loading

When it comes to the #![feature(const_fn_trait_bound)] gate, I did not add it myself. Can I assume that it's fine to keep Option::copied in the PR or should it be removed?

@lambinoo lambinoo force-pushed the 67441-const-fn-copied-take-replace branch from 7b1bc53 to b08303e Compare July 3, 2021 14:20
wooster0
wooster0 suggested changes Jul 3, 2021
View reviewed changes
library/core/src/option.rs Show resolved Hide resolved
library/core/src/option.rs Show resolved Hide resolved
@lambinoo lambinoo force-pushed the 67441-const-fn-copied-take-replace branch from a1f5d69 to fac9281 Compare July 3, 2021 16:11
fee1-dead
fee1-dead reviewed Jul 3, 2021
View reviewed changes
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
@lambinoo lambinoo force-pushed the 67441-const-fn-copied-take-replace branch 3 times, most recently from 2f75b1b to 42447dd Compare July 3, 2021 17:02
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2021
@lambinoo
Copy link
Contributor Author

Hey! Any updates on this?

@bors
Copy link
Contributor

bors commented Aug 5, 2021

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

@jackh726 jackh726 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 22, 2021
@lambinoo
const fn for option copied, take & replace + tests
10ddabc 
fix: move test that require mut to another

Adding TODOs for Option::take and Option::copied

TODO to FIXME + moving const stability under normal

Moving const stability attr under normal stab attr

move more rustc stability attributes
@lambinoo lambinoo force-pushed the 67441-const-fn-copied-take-replace branch from 42447dd to 10ddabc Compare August 29, 2021 11:19
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2021
@JohnCSimon
Copy link
Member

triage:

Hey! Any updates on this?

@lambinoo Josh Triplett seems to have a large review backlog.

@joshtriplett
Copy link
Member

Sorry for the delayed review.

This seems reasonable to me, particularly since all of these are rustc_const_unstable.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2021

📌 Commit 10ddabc has been approved by joshtriplett

@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 Oct 4, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
@Manishearth
Rollup merge of rust-lang#86828 - lambinoo:67441-const-fn-copied-take…
1c4129d 
…-replace, r=joshtriplett

const fn for option copied, take & replace

Tracking issue: [rust-lang#67441](rust-lang#67441)

Adding const fn for the copied, take and replace method of Option. Also adding necessary unit test.

It's my first contribution so I am pretty sure I don't know what I'm doing but there's a first for everything!
This was referenced Oct 4, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2021
@bors
Auto merge of rust-lang#89512 - Manishearth:rollup-meh9x7r, r=Manishe…
44593ae 
…arth

Rollup of 14 pull requests

Successful merges:

 - rust-lang#86434 (Add `Ipv6Addr::is_benchmarking`)
 - rust-lang#86828 (const fn for option copied, take & replace)
 - rust-lang#87679 (BTree: refine some comments)
 - rust-lang#87910 (Mark unsafe methods NonZero*::unchecked_(add|mul) as const.)
 - rust-lang#88286 (Remove unnecessary unsafe block in `process_unix`)
 - rust-lang#88305 (Manual Debug for Unix ExitCode ExitStatus ExitStatusError)
 - rust-lang#88353 (Partially stabilize `array_methods`)
 - rust-lang#88370 (Add missing `# Panics` section to `Vec` method)
 - rust-lang#88481 (Remove some feature gates)
 - rust-lang#89138 (Fix link in Ipv6Addr::to_ipv4 docs)
 - rust-lang#89401 (Add truncate note to Vec::resize)
 - rust-lang#89467 (Fix typos in rustdoc/lints)
 - rust-lang#89472 (Only register `WSACleanup` if `WSAStartup` is actually ever called)
 - rust-lang#89505 (Add regression test for spurious const error with NLL)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 287af04 into rust-lang:master Oct 4, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fee1-dead fee1-dead fee1-dead left review comments

@wooster0 wooster0 wooster0 requested changes

Assignees

@joshtriplett joshtriplett

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

Successfully merging this pull request may close these issues.