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

Changes to naive::internals to enable more const functions #882

Closed
wants to merge 17 commits into from

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Nov 19, 2022

Not ready for review yet. I've just pushed the code to enable some discussions on #815

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 20, 2022

Using

struct DateImpl {
    year: i16,
    month: Month,
    day: NonZeroU8,
}

might be another option here, especially as requesting the day and month are probably more common than the ordinal.

Either option also resolves #811 for NaiveDate

@djc
Copy link
Member

djc commented Nov 20, 2022

@esheppa you alternative DateImpl formulation might be attractive in some ways, just want to point out that it would substantially tighten the available range, since the year range goes from 19 bits to 16. We might decide that's a decent trade-off, just pointing it out. (And probably also means this shouldn't go in 0.4.)

@esheppa
Copy link
Collaborator Author

esheppa commented Nov 23, 2022

Yes I definitely thing this should be a 0.5+ thing, it would be very painful to implement at MSRV < 1.48. Also we wouldn't get the benefit of having less fallible APIs unless we can change the year parameters to i16. My main intention of this is to start a conversation around whether it is worth narrowing the date range. There are also other tradeoffs here, for example using calculations instead of lookup tables, which worsen performance, but allow validation at compile time when using const generic arguments.

Interestingly the impl with month and day fields has even worse performance, so I need to do a bit more investigation of why that is.

@pitdicker
Copy link
Collaborator

@esheppa You did a lot of work here. In #1043 (comment) you thought that there are parts here that can be useful since the alternative approach in #1043 was merged?

@pitdicker
Copy link
Collaborator

Closing for now. Any follow up work is probably going to be a new PR anyway.

@pitdicker pitdicker closed this Jul 17, 2023
@pitdicker pitdicker deleted the fallible-of-ext-v2 branch February 13, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants