-
Notifications
You must be signed in to change notification settings - Fork 515
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
Making Duration::seconds and similar functions const #638
Conversation
This commit updates `Duration::seconds` and siblings to be const. This requires the const-panic api that only stabilized in 1.57, so it may be best to hide this behid a feature flag somehow so as to not require a MSRV of 1.57. This is my first pr so I don't know the policy around this. This pr also adds a bit of mess to the functions because we can't use `.expect("error")` or `Datetime::partial_cmp`. I think it is worth it because const durations would be really nice to have.
This will make `from_std` const. We can't make `to_std` const until `std::time::Duration::new` stabilizes as a const function.
It seems there's an error with lint. It doesn't finish for months. |
As mentioned in #309, this will need to be conditional on an opt-in feature flag, otherwise we have to raise the MSRV too fast. Once that change is made I'm open to merging something like this. |
I'm pretty busy this week. But this coming weekend I'll figure out how to do a opt-in feature flag and make the change. |
@djc Would you consider a dependency on a crate like rustversion or const_fn? |
I'd prefer not to take on a procedural macro dependency. Maybe a declarative macro will work for this, something that can be called with an optional token for the |
Has this stagnated? I'd love to have this feature. |
There has been no progress as far as I know. @msdrigg did you want to move this forward? @mattfbacon if not, would you be willing to open a new PR based on this branch and incorporate the review feedback? |
I dont think I will have time for this for the foreseeable future. If @mattfbacon can take the reigns, it would be much appreciated |
Sure, I could take a look. I'll try to figure out if a |
Const on debug, non-const on release. I couldn't figure out how to support |
As another way forward here, potentially we could have a "maybe-const" feature which then activates a dependency on If we want to use a |
Yeah, not super excited about pulling in a dependency or a build script for this stuff. |
As an extra note, I'm experimenting with ways to use @mattfbacon / @msdrigg - would having a |
core::time::Duration doesn't support negative Durations, right? I thought there was a fairly fundamental difference. |
Correct - I'm trying out two options: pub enum SignedDuration {
Forwards(Duration),
Backwards(Duration),
} once I've got all the tests passing, I'll create some PR's to collect feedback on whether any of those might be feasible, or if we can potentially find a path forward from one of those options to something feasible. |
I played with this PR, hoping to help push it over the finish line now that our MSRV is 1.56 and 1.57 is maybe possible. But I now think it is not good to do this in the 0.4.x branch. We can only make the methods const for our own implementation, which you get with the Suppose you use the const methods in your crate. When you add a dependency that somewhere in its dependency graph depends on chrono and doesn't set the flag, your crate suddenly fails to compile. The situation where enabling a feature flag makes less code compile does not seem smart. |
Our MSRV is now 1.57, and we no longer have the time 0.1 dependency. So this change has become possible. #1137 adds @msdrigg I have included you Thank you for working on this! |
Glad to see the progress on this! |
This commit updates
Duration::seconds
and siblings to be const. Thisrequires the const-panic api that only stabilized in 1.57, so it may be
best to hide this behid a feature flag somehow so as to not require a
MSRV of 1.57. This is my first pr so I don't know the policy around
this.
This pr also adds a bit of mess to the functions because we can't use
.expect("error")
orDatetime::partial_cmp
. I think it is worth itbecause const durations would be really nice to have.
This also resolves #309