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

Make some Option, Result methods unstably const #82130

Merged
merged 1 commit into from
Mar 7, 2021
Merged

Make some Option, Result methods unstably const #82130

merged 1 commit into from
Mar 7, 2021

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Feb 15, 2021

The following methods are now unstably const:

  • Option::transpose
  • Option::flatten
  • Result::flatten

While some methods for could likely be made const in the future, nearly all of them require something to be dropped at compile-time, which isn't currently supported. The functions listed above should have a trivial path to stabilization.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2021
@jhpratt

This comment has been minimized.

@rustbot rustbot added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 15, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@CDirkx
Copy link
Contributor

CDirkx commented Feb 15, 2021

The functions listed above should have a trivial path to stabilization.

These functions implicitly depend on const_precise_live_drops, without which the compiler complains that something might be dropped (even though it isn't). I believe these functions thus cannot be stabilized before const_precise_live_drops is stabilized (#76654 (comment)).

@joshtriplett
Copy link
Member

r? @ecstatic-morse

@JohnTitor
Copy link
Member

Triage: @ecstatic-morse seems busy recently, r? @RalfJung could you take a look?

library/core/src/result.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Mar 5, 2021

This LGTM; Cc @rust-lang/wg-const-eval

@jhpratt
Copy link
Member Author

jhpratt commented Mar 5, 2021

With regard to the const-hack label, should we really consider this a hack? I just quickly checked on godbolt, and it looks like the new implementation actually optimizes slightly better at lower opt levels. Not a huge deal either way, but I figure we may as well take a minuscule performance gain where we can. The implementation is straightforward in either situation.

@RalfJung
Copy link
Member

RalfJung commented Mar 5, 2021

We typically use that label when code gets changed to be const-compatible. I don't feel competent to evaluate whether the old or the new implementation is better, but I can imagine that avoiding higher-order functions helps. @oli-obk what do you think?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 6, 2021

I actually prefer the new implementation since imo it's more readable, even if the old version is more compact

library/core/src/result.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2021

I actually prefer the new implementation since imo it's more readable, even if the old version is more compact

All right, I tend to agree so I removed the label.

The following functions are now unstably const:

- Option::transpose
- Option::flatten
- Result::transpose
@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 6, 2021

📌 Commit 79c2b75 has been approved by RalfJung

@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 6, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 6, 2021
…Jung

Make some Option, Result methods unstably const

The following methods are now unstably const:

- Option::transpose
- Option::flatten
- Result::flatten

While some methods for could likely be made `const` in the future, nearly all of them require something to be dropped at compile-time, which isn't currently supported. The functions listed above should have a trivial path to stabilization.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2021
Rollup of 13 pull requests

Successful merges:

 - rust-lang#77916 (Change built-in kernel targets to be os = none throughout)
 - rust-lang#82130 (Make some Option, Result methods unstably const)
 - rust-lang#82292 (Prevent specialized ZipImpl from calling `__iterator_get_unchecked` twice with the same index)
 - rust-lang#82402 (Remove RefCell around `module_trait_cache`)
 - rust-lang#82592 (Improve transmute docs with further clarifications)
 - rust-lang#82651 (Cleanup rustdoc warnings)
 - rust-lang#82720 (Fix diagnostic suggests adding type `[type error]`)
 - rust-lang#82751 (improve offset_from docs)
 - rust-lang#82793 (Move some tests to more suitable subdirs)
 - rust-lang#82803 (rustdoc: Add an unstable option to print all unversioned files)
 - rust-lang#82808 (Sync rustc_codegen_cranelift)
 - rust-lang#82822 (Fix typo)
 - rust-lang#82837 (tweak MaybeUninit docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0adc196 into rust-lang:master Mar 7, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. 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
Development

Successfully merging this pull request may close these issues.