-
Notifications
You must be signed in to change notification settings - Fork 75
Replace ConstCtOption with ctutils::CtOption
#1040
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1040 +/- ##
==========================================
+ Coverage 79.13% 79.21% +0.07%
==========================================
Files 165 165
Lines 17616 17520 -96
==========================================
- Hits 13940 13878 -62
+ Misses 3676 3642 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cf97fff to
fe39194
Compare
ConstCtOption with ctutils::CtOptionConstCtOption with ctutils::CtOption
|
As a bit of a plan of where to go from here: I'd like to get rid of For now we can impl both the I'd like to replace anything which isn't a |
|
This will also make it possible to get rid of the |
Analogous to what #1035 did for `ConstChoice`, this extracts the necessary methods onto `ctutils::CtOption` then replaces the type wholesale using the implementation in `ctutils`. There were a few tricky parts, namely we had type-specific implementations of `expect` and `unwrap_or`. It is actually possible to write a generic `const fn` implementation of `CtOption::expect`, avoiding the problems with destructors and the unstable `Destruct` trait, by bounding on `T: Copy` and returning a copy of the inner value. This is implemented as `CtOption::expect_copied` so we can still support `expect` for non-`Copy` types. Unfortunately there's no analogous generic `const fn` solution for `CtOption::unwrap_or`, as this needs a constant-time selector function and even function pointers can't be used from a `const fn`. So this does the only thing we can do to replace it with some level of abstraction: a `ctutils::unwrap_or!` macro. See also: RustCrypto/utils#1274 which extracts some needed methods onto `ctutils::CtOption`.
fe39194 to
a9c1882
Compare
|
Benchmarks are, as usual, all over the place. I've rerun them a few times to see if there's anything that sticks out as consistently better or worse, and it just seems like especially for operations that are measured in I've seen a few things that feel like flukes (e.g. All that said, I'm not seeing anything particularly concerning. |
|
Given that, I'm going to go ahead and merge, and press forward trying to get rid of the |
Analogous to what #1035 did for
ConstChoice, this extracts the necessary methods ontoctutils::CtOptionthen replaces the type wholesale using the implementation inctutils.There were a few tricky parts, namely we had type-specific implementations of
expectandunwrap_or.It is actually possible to write a generic
const fnimplementation ofCtOption::expect, avoiding the problems with destructors and the unstableDestructtrait, by bounding onT: Copyand returning a copy of the inner value. This is implemented asCtOption::expect_copiedso we can still supportexpectfor non-Copytypes.Unfortunately there's no analogous generic
const fnsolution forCtOption::unwrap_or, as this needs a constant-time selector function and even function pointers can't be used from aconst fn. So this does the only thing we can do to replace it with some level of abstraction: actutils::unwrap_or!macro.See also: RustCrypto/utils#1274 which extracts some needed methods onto
ctutils::CtOption.cc @andrewwhitehead