-
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
Make functions in internals const #1043
Conversation
da835ae
to
9167548
Compare
@@ -53,7 +51,7 @@ pub(super) const FE: YearFlags = YearFlags(0o07); | |||
pub(super) const G: YearFlags = YearFlags(0o16); | |||
pub(super) const GF: YearFlags = YearFlags(0o06); | |||
|
|||
static YEAR_TO_FLAGS: [YearFlags; 400] = [ | |||
const YEAR_TO_FLAGS: &[YearFlags; 400] = &[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A static is only included once in the binary. To prevent duplication of this array on every use as const, It has to be put behind a reference (see https://users.rust-lang.org/t/const-vs-static/52951/12). It can still be duplicated with multiple codegen units, but the statics are not that big that it should be a problem.
let mdl = mdf >> 3; | ||
match MDL_TO_OL.get(mdl as usize) { | ||
Some(&v) => Of(mdf.wrapping_sub((i32::from(v) as u32 & 0x3ff) << 3)), | ||
None => Of(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three lines where using array.get()
and returning a safe value instead of panicking on out-of-bounds. Yet the index value is created in this module, and always in bounds.
I suspect the reason here is not just to avoid a potential panic, but to avoid the creation of landings pads if the compiler can't elide the bounds check.
In my opinion it would be fine to simplify this code to direct array indexing, but I kept the functionality. Note that array.get()
is unavailable in const, so this needs a manual bounds check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ai. I was wrong, this code is just ancient. In various places it is returning 0
instead of Option
, and you are supposed to call valid()
afterwards 😞. Let me see if I can clean that up a little.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may even have some correctness gains by separating these values out to separate modules, with the modules only exposing APIs that allow valid values (in cases where a const API still requires things like .valid())
@@ -368,33 +370,37 @@ impl fmt::Debug for Of { | |||
/// (month, day of month and leap flag), | |||
/// which is an index to the `MDL_TO_OL` lookup table. | |||
#[derive(PartialEq, PartialOrd, Copy, Clone)] | |||
pub(super) struct Mdf(pub(super) u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value didn't need to be public. Making it private makes it clearer the value is always correct, and thus in bounds for array indexing.
src/naive/internals.rs
Outdated
let Of(of) = *self; | ||
Weekday::from_u32(((of >> 4) + (of & 0b111)) % 7).unwrap() | ||
Weekday::from_u32_mod7((of >> 4) + (of & 0b111)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use Weekday::from_u32
because that is part of num_traits::FromPrimitive
, and trait methods can't yet be const.
src/weekday.rs
Outdated
/// Create a `Weekday` from an `u32`, with Monday = 0. | ||
/// Infallible, takes `n & 7`. | ||
#[inline] | ||
pub(crate) const fn from_u32_mod7(n: u32) -> Weekday { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For use in this crate it would be nice if there was a const, infallible way to create a Weekday
.
Because the two methods constructing a Weekday
in internals.rs
passed n % 7
, making from_u32_mod7
seemed like a good way to have an infallible function and avoid a little code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the mod7
requirement which is not enforced in the type system and the fact that this is used only in the internals
module, I think this should be a free function in the internals
module rather than a method here.
@RagibHasin You expressed an interest to help with making the API const. Would you also be willing to review? And maybe we can work together? It seems hard to avoid one massive 'make everything const'-PR, because a lot depends on each other.
|
Thanks, @pitdicker. I expect to have some free time in around 12 hours. I would try to review it then. Very excited about this progress. |
#638 Already worked on Maybe we can use some ugly macro that imitates them but works in const by doing an implicit panic, like https://users.rust-lang.org/t/compile-time-const-unwrapping/51619/6? Or chrono would have to update to Rust 2021 and raise the MSRV to 1.57, which supports panic in const context. See #995. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a safe and useful improvement!
Just one request for a new test case.
6540cbf
to
459defd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While making parts of NaiveDate
const, I noticed there where Option
s created in places where I would not expect them, and not created where they should.
Of
was written to allow the creation of invalid values, and you were supposed to call valid()
afterwards and handle the error. That didn't seem very Rusty, so I changed Of
is to always be valid and return Option
s earlier.
There is not any extra validation happening. It is just happening in the functions that would create the invalid state, not in the functions calling them.
/// Does not check whether the flags are correct for the provided year. | ||
const fn from_of(year: i32, ordinal: u32, flags: YearFlags) -> Option<NaiveDate> { | ||
if year < MIN_YEAR || year > MAX_YEAR { | ||
return None; // Out-of-range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already splits out the failure causes for when this method is switched to return a Result
.
src/naive/date.rs
Outdated
|
||
/// Makes a new `NaiveDate` from year, ordinal and flags. | ||
/// Does not check whether the flags are correct for the provided year. | ||
const fn from_of(year: i32, ordinal: u32, flags: YearFlags) -> Option<NaiveDate> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was always used in a pattern of NaiveDate::from_of(year, Of::new(ordinal, flags)?)
. Clearer to combine them, which helps to see the failure cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This combination should be a separate commit with no other changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also rename this method? The current name doesn't really fit anymore.
} | ||
match mdf.to_of() { | ||
Some(of) => Some(NaiveDate { ymdf: (year << 13) | (of.inner() as DateImpl) }), | ||
None => None, // Non-existing date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return another error than in from_of
if this methods is converted to return a Result
.
} else { | ||
None | ||
} | ||
const fn with_of(&self, of: Of) -> NaiveDate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because an Of
is now always valid, this function no longer has to return Option
.
@@ -172,8 +172,9 @@ impl fmt::Debug for YearFlags { | |||
} | |||
} | |||
|
|||
// OL: (ordinal << 1) | leap year flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the important trick of the validation logic.
@@ -266,56 +267,66 @@ const OL_TO_MDL: &[u8; MAX_OL as usize + 1] = &[ | |||
/// The whole bits except for the least 3 bits are referred as `Ol` (ordinal and leap flag), | |||
/// which is an index to the `OL_TO_MDL` lookup table. | |||
#[derive(PartialEq, PartialOrd, Copy, Clone)] | |||
pub(super) struct Of(pub(crate) u32); | |||
pub(super) struct Of(u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of Of
is now private and always valid.
} | ||
|
||
pub(super) const fn from_date_impl(date_impl: DateImpl) -> Of { | ||
// We assume the value in the `DateImpl` is valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add this to make Of
private, and prevent unnecessary validation when converting from DateImpl
. But DateImpl
is not private... Had to stop somewhere 😄
Thanks @pitdicker - this looks great. Looks like there were a few critical parts I missed that led me down the path of an internals rewrite (that said, some of it was to try to simplify the code and avoid some conversions, but in the end the benchmarks were much worse). The good news is this looks like a great solution, and I can potentially look into doing a new PR with some of the potentially useful parts of #882 once this is merged |
With regards to:
What we can do here is have a non-default feature that allows this (eg |
|
Yes I think the best solution is to avoid negative durations - however I'm also keen to receive any feedback on where they would be useful. The current design is implemented in this PR: #895 (a continuation of a long series of designs, but this one is able to be used in the Originally we had thought to use a new enum, eg: enum TimeDelta { Forwards(Duration), Backwards(Duration) } However in the current iteration I'm just reusing the @RagibHasin as this is off topic for this PR, we should continue any further discussions on this at #895 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At risk of sounding like a broken record...
Could some 1:1 tests be added, e.g. each function
gets it's own test_function
?
On top of that, preferably added in two or more commits:
- A bunch of tests in one (or more) a before changes commit
- and then one (or more) commit with changes + test adjustments.
The additional tests provide clarity to developers about function expectations (esp. code reviewers 😊) and give more confidence about the changes.
Just my request. I'm requesting this now and not previously because a new commit has added a few behavior changes (459defd).
Also, just having more 1:1 function testing would be good for chrono
.
Please do! I'll learn 😄 For these changes though: this code is already tested really, really thoroughly.
I don't see much to improve here w.r.t the existing tests. Or did you mean: add tests to verify the functions keep working in const context? |
Oops, replied to soon. I'll add tests that verify the changed functions return |
Nice! Seems like most of the code necessary to make functions in chrono const already exists, it just has to come together 👍. |
Is that something that can be done on the 0.4.x branch, or would it require breaking changes? I'll reply on your #895 when I get to know that area a little better. Maybe for the somewhat simple work of making methods const we can make something work for the current implementation.
I remember that removing a feature can only be done when going to a new major version. I.e. if we introduce it in 0.4.x, it can't be removed until 0.5. That doesn't seem too bad... Maybe a more general This feels like a ok-ish solution to me. A macro that maps to regular Maybe just work on that assumption for now, and make a PR with the workarounds if the MSRV has to stay below 1.57? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry for the slow follow-up. I let myself get distracted by some very interesting panics (in another issue)... Added a test to check Thank you for the reminders to add tests @jtmoon79. |
bfc85fc
to
600c953
Compare
src/naive/internals.rs
Outdated
match MDL_TO_OL.get(mdl as usize) { | ||
Some(&v) => Of(mdf.wrapping_sub((i32::from(v) as u32 & 0x3ff) << 3)), | ||
None => Of(0), | ||
if mdl <= MAX_MDL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure <=
is incorrect when preparing to index into something? Also for cases like these I prefer to invert the condition so we can do an early return for the simple case (here, Of(0)
) and resume unindented for the straight-line path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arrays in this file are a bit strange: index 0
has an invalid value, and the length is MAX + 1
.
It makes some sense, as we encode ordinals and day of month starting from 1. This allows indexing without the need to subtract -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, can you add a comment to each comparison where this is relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like the comment on the same line as the if
, can be shorter like // 1-based indexing
.
match OL_TO_MDL.get(ol as usize) { | ||
Some(&v) => Mdf(of + (u32::from(v) << 3)), | ||
None => Mdf(0), | ||
if ol <= MAX_OL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be <
? Please add some tests for the edge cases here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same array with an unused zero index. There is a test with the edge cases.
match MDL_TO_OL.get(mdl as usize) { | ||
Some(&v) => v >= 0, | ||
None => false, | ||
if mdl <= MAX_MDL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<=
-> <
?
src/weekday.rs
Outdated
/// Create a `Weekday` from an `u32`, with Monday = 0. | ||
/// Infallible, takes `n & 7`. | ||
#[inline] | ||
pub(crate) const fn from_u32_mod7(n: u32) -> Weekday { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the mod7
requirement which is not enforced in the type system and the fact that this is used only in the internals
module, I think this should be a free function in the internals
module rather than a method here.
src/naive/date.rs
Outdated
|
||
/// Makes a new `NaiveDate` from year, ordinal and flags. | ||
/// Does not check whether the flags are correct for the provided year. | ||
const fn from_of(year: i32, ordinal: u32, flags: YearFlags) -> Option<NaiveDate> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This combination should be a separate commit with no other changes.
src/weekday.rs
Outdated
@@ -260,6 +260,14 @@ mod tests { | |||
); | |||
} | |||
} | |||
|
|||
#[test] | |||
fn test_from_u32_mod7() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash this in the commit where the function is added.
@@ -266,6 +266,8 @@ const OL_TO_MDL: &[u8; MAX_OL as usize + 1] = &[ | |||
/// | |||
/// The whole bits except for the least 3 bits are referred as `Ol` (ordinal and leap flag), | |||
/// which is an index to the `OL_TO_MDL` lookup table. | |||
/// | |||
/// The methods implemented on `Of` always return a valid value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash this change into the commit that originated the described changes.
59122a3
to
b354578
Compare
b354578
to
013427e
Compare
src/naive/internals.rs
Outdated
match MDL_TO_OL.get(mdl as usize) { | ||
Some(&v) => Of(mdf.wrapping_sub((i32::from(v) as u32 & 0x3ff) << 3)), | ||
None => Of(0), | ||
if mdl <= MAX_MDL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like the comment on the same line as the if
, can be shorter like // 1-based indexing
.
src/naive/internals.rs
Outdated
@@ -467,11 +466,28 @@ impl fmt::Debug for Mdf { | |||
} | |||
} | |||
|
|||
/// Create a `Weekday` from an `u32`, with Monday = 0. | |||
/// Infallible, takes `n % 7`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The takes n % 7
comment is no longer correct, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was to say it is infallible, because it takes any n
and does modulus 7. I'll see if I can word it better.
for i in 0..=1000 { | ||
assert_eq!(weekday_from_u32_mod7(i), Weekday::from_u32(i % 7).unwrap()); | ||
} | ||
assert_eq!(weekday_from_u32_mod7(u32::MAX), Weekday::Thu); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any point to testing with values larger than 7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, but testing up to 1000 and a single u32:MAX
is cheap.
src/naive/internals.rs
Outdated
if self.ordinal() == 1 { | ||
return None; | ||
} | ||
Some(Of(self.0 - (1 << 4))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: I prefer writing this as
match self.ordinal() {
1 => None,
_ => Some(Of(self.0 - (1 << 4)))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks very clean, thank you 👍
src/naive/internals.rs
Outdated
@@ -370,13 +387,17 @@ impl fmt::Debug for Of { | |||
/// The whole bits except for the least 3 bits are referred as `Mdl` | |||
/// (month, day of month and leap flag), | |||
/// which is an index to the `MDL_TO_OL` lookup table. | |||
/// | |||
/// The methods implemented on `Mdf` do not always return a valid value. | |||
/// Dates than can't exist, like February 30, can still be represented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"than" -> "that"
Why do we need to allow Mdf
instances with an invalid value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing whether an ordinal is valid is easy. Testing a month, day, leap year flag combination is valid, and doing it fast, is more involved.
The table lookup to convert Mdf
to Of
is pretty much the ideal place to put the validity check.
@@ -662,8 +683,7 @@ mod tests { | |||
fn test_of_fields() { | |||
for &flags in FLAGS.iter() { | |||
for ordinal in range_inclusive(1u32, 366) { | |||
let of = Of::new(ordinal, flags).unwrap(); | |||
if of.valid() { | |||
if let Some(of) = Of::new(ordinal, flags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tests, let's just unwrap()
cases like these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't. The test is creating all possible combinations of year flags and ordinals. But some of these combinations are just invalid. It tests if those that are let through, have an ordinal that round-trips.
src/naive/date.rs
Outdated
|
||
/// Makes a new `NaiveDate` from year, ordinal and flags. | ||
/// Does not check whether the flags are correct for the provided year. | ||
const fn from_of(year: i32, ordinal: u32, flags: YearFlags) -> Option<NaiveDate> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also rename this method? The current name doesn't really fit anymore.
I would also like to put it on the same line, but rustfmt disagrees with me. |
013427e
to
2e71e97
Compare
(Some of your comments are missing a reply field for some reason.) I suppose it can be called |
@djc Just to make sure, you are not waiting on me? |
Nope. |
2e71e97
to
d2e88b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for working on this!
🎉Time to get my branch that makes most of |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [chrono](https://github.com/chronotope/chrono) | dependencies | patch | `0.4.24` -> `0.4.26` | --- ### Release Notes <details> <summary>chronotope/chrono</summary> ### [`v0.4.26`](https://github.com/chronotope/chrono/releases/tag/v0.4.26): 0.4.26 [Compare Source](chronotope/chrono@v0.4.25...v0.4.26) The changes from [#​807](chronotope/chrono#807) we merged for 0.4.25 unfortunately restricted parsing in a way that was incompatible with earlier 0.4.x releases. We reverted this in [#​1113](chronotope/chrono#1113). A small amount of other changes were merged since. - Update README ([#​1111](chronotope/chrono#1111), thanks to [@​pitdicker](https://github.com/pitdicker)) - Revert backport of [#​807](chronotope/chrono#807) ([#​1113](chronotope/chrono#1113), thanks to [@​pitdicker](https://github.com/pitdicker)) - Update to 2021 edition ([#​1109](chronotope/chrono#1109), thanks to [@​tottoto](https://github.com/tottoto)) - Fix `DurationRound` panics from issue [#​1010](chronotope/chrono#1010) ([#​1093](chronotope/chrono#1093), thanks to [@​pitdicker](https://github.com/pitdicker)) - tests: date path consolidate (branch 0.4.x) ([#​1090](chronotope/chrono#1090), thanks to [@​jtmoon79](https://github.com/jtmoon79)) - Parse tests nanosecond bare dot (branch 0.4.x) ([#​1098](chronotope/chrono#1098), thanks to [@​jtmoon79](https://github.com/jtmoon79)) - yamllint cleanup lint.yml test.yml ([#​1102](chronotope/chrono#1102), thanks to [@​jtmoon79](https://github.com/jtmoon79)) - Remove num-iter dependency ([#​1107](chronotope/chrono#1107), thanks to [@​tottoto](https://github.com/tottoto)) Thanks on behalf of the chrono team ([@​djc](https://github.com/djc) and [@​esheppa](https://github.com/esheppa)) to all contributors! ### [`v0.4.25`](https://github.com/chronotope/chrono/releases/tag/v0.4.25): 0.4.25 [Compare Source](chronotope/chrono@v0.4.24...v0.4.25) Time for another maintenance release. This release bumps the MSRV to 1.56; given MSRV bumps in chrono's dependencies (notably for syn 2), we felt that it no longer made sense to support any older versions. Feedback welcome in our issue tracker! ##### Additions - Bump the MSRV to 1.56 ([#​1053](chronotope/chrono#1053)) - Apply comments from MSRV bump ([#​1026](chronotope/chrono#1026), thanks to [@​pitdicker](https://github.com/pitdicker)) - Remove num-integer dependency ([#​1037](chronotope/chrono#1037), thanks to [@​pitdicker](https://github.com/pitdicker)) - Add `NaiveDateTime::and_utc()` method ([#​952](chronotope/chrono#952), thanks to [@​klnusbaum](https://github.com/klnusbaum)) - derive `Hash` for most pub types that also derive `PartialEq` ([#​938](chronotope/chrono#938), thanks to [@​bruceg](https://github.com/bruceg)) - Add `parse_and_remainder()` methods ([#​1011](chronotope/chrono#1011), thanks to [@​pitdicker](https://github.com/pitdicker)) - Add `DateTime::fix_offset()` ([#​1030](chronotope/chrono#1030), thanks to [@​pitdicker](https://github.com/pitdicker)) - Add `#[track_caller]` to `LocalResult::unwrap` ([#​1046](chronotope/chrono#1046), thanks to [@​pitdicker](https://github.com/pitdicker)) - Add `#[must_use]` to some methods ([#​1007](chronotope/chrono#1007), thanks to [@​aceArt-GmbH](https://github.com/aceArt-GmbH)) - Implement `PartialOrd` for `Month` ([#​999](chronotope/chrono#999), thanks to [@​Munksgaard](https://github.com/Munksgaard)) - Add `impl From<NaiveDateTime> for NaiveDate` ([#​1012](chronotope/chrono#1012), thanks to [@​pezcore](https://github.com/pezcore)) - Extract timezone info from tzdata file on Android ([#​978](chronotope/chrono#978), thanks to [@​RumovZ](https://github.com/RumovZ)) ##### Fixes - Prevent string slicing inside char boundaries ([#​1024](chronotope/chrono#1024), thanks to [@​pitdicker](https://github.com/pitdicker)) - fix IsoWeek so that its flags are always correct ([#​991](chronotope/chrono#991), thanks to [@​moshevds](https://github.com/moshevds)) - Fix out-of-range panic in `NaiveWeek::last_day` ([#​1070](chronotope/chrono#1070), thanks to [@​pitdicker](https://github.com/pitdicker)) - Use correct offset in conversion from `Local` to `FixedOffset` ([#​1041](chronotope/chrono#1041), thanks to [@​pitdicker](https://github.com/pitdicker)) - Fix military timezones in RFC 2822 parsing ([#​1013](chronotope/chrono#1013), thanks to [@​pitdicker](https://github.com/pitdicker)) - Guard against overflow in NaiveDate::with_\*0 methods ([#​1023](chronotope/chrono#1023), thanks to [@​pitdicker](https://github.com/pitdicker)) - Fix panic in from_num_days_from_ce_opt ([#​1052](chronotope/chrono#1052), thanks to [@​pitdicker](https://github.com/pitdicker)) ##### Refactoring - Rely on std for getting local time offset ([#​1072](chronotope/chrono#1072), thanks to [@​pitdicker](https://github.com/pitdicker)) - Make functions in internals const ([#​1043](chronotope/chrono#1043), thanks to [@​pitdicker](https://github.com/pitdicker)) - Refactor windows module in `Local` ([#​992](chronotope/chrono#992), thanks to [@​nekevss](https://github.com/nekevss)) - Simplify from_timestamp_millis, from_timestamp_micros ([#​1032](chronotope/chrono#1032), thanks to [@​pitdicker](https://github.com/pitdicker)) - Backport [#​983](chronotope/chrono#983) and [#​1000](chronotope/chrono#1000) ([#​1063](chronotope/chrono#1063), thanks to [@​pitdicker](https://github.com/pitdicker)) ##### Documentation - Backport documentation improvements ([#​1066](chronotope/chrono#1066), thanks to [@​pitdicker](https://github.com/pitdicker)) - Add documentation for %Z quirk ([#​1051](chronotope/chrono#1051), thanks to [@​campbellcole](https://github.com/campbellcole)) - Add an example to Weekday ([#​1019](chronotope/chrono#1019), thanks to [@​pitdicker](https://github.com/pitdicker)) ##### Internal improvements - Gate test on `clock` feature ([#​1061](chronotope/chrono#1061), thanks to [@​pitdicker](https://github.com/pitdicker)) - CI: Also run tests with `--no-default-features` ([#​1059](chronotope/chrono#1059), thanks to [@​pitdicker](https://github.com/pitdicker)) - Prevent `bench_year_flags_from_year` from being optimized out ([#​1034](chronotope/chrono#1034), thanks to [@​pitdicker](https://github.com/pitdicker)) - Fix test_leap_second during DST transition ([#​1064](chronotope/chrono#1064), thanks to [@​pitdicker](https://github.com/pitdicker)) - Fix warnings when running tests on Windows ([#​1038](chronotope/chrono#1038), thanks to [@​pitdicker](https://github.com/pitdicker)) - Fix tests on AIX ([#​1028](chronotope/chrono#1028), thanks to [@​ecnelises](https://github.com/ecnelises)) - Fix doctest warnings, remove mention of deprecated methods from main doc ([#​1081](chronotope/chrono#1081), thanks to [@​pitdicker](https://github.com/pitdicker)) - Reformat `test_datetime_parse_from_str` ([#​1078](chronotope/chrono#1078), thanks to [@​pitdicker](https://github.com/pitdicker)) - GitHub yml shell `set -eux`, use bash ([#​1103](chronotope/chrono#1103), thanks to [@​jtmoon79](https://github.com/jtmoon79)) - test: explicitly set `LANG` to `c` in gnu `date` ([#​1089](chronotope/chrono#1089), thanks to [@​scarf005](https://github.com/scarf005)) - Switch test to `TryFrom` ([#​1086](chronotope/chrono#1086), thanks to [@​pitdicker](https://github.com/pitdicker)) - Add test for issue 551 ([#​1020](chronotope/chrono#1020), thanks to [@​pitdicker](https://github.com/pitdicker)) - RFC 2822 single-letter obsolete tests ([#​1014](chronotope/chrono#1014), thanks to [@​jtmoon79](https://github.com/jtmoon79)) - \[CI] Lint Windows target and documentation links ([#​1062](chronotope/chrono#1062), thanks to [@​pitdicker](https://github.com/pitdicker)) - add test_issue\_866 ([#​1077](chronotope/chrono#1077), thanks to [@​jtmoon79](https://github.com/jtmoon79)) - Remove AUTHORS metadata ([#​1074](chronotope/chrono#1074)) On behalf of [@​djc](https://github.com/djc) and [@​esheppa](https://github.com/esheppa), thanks to all contributors! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMDUuMSIsInVwZGF0ZWRJblZlciI6IjM1LjExNS4yIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1909 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@esheppa My intention was to understand why you would need to overhaul the internals of chono in #882 to make functions const. So I started sprinkling
const
around to see what breaks.Everything in
internals.rs
can easily be made const, even on the 0.4.x branch. I made this PR because the work was done anyway, no use in letting someone else do it again.@esheppa Would you be willing to review?
I also did a good number of the functions from
NaiveDate
just to prove for myself there are no major obstacles, but kept them out of this PR to keep it reviewable. There are a lot more choices that can be made there, so they deserve an own PR in the future.I'll add some comments on the required changes.