jiff: Days in month rangeint should always have range 28-31#85
Merged
BurntSushi merged 1 commit intoBurntSushi:masterfrom Aug 8, 2024
Merged
jiff: Days in month rangeint should always have range 28-31#85BurntSushi merged 1 commit intoBurntSushi:masterfrom
BurntSushi merged 1 commit intoBurntSushi:masterfrom
Conversation
BurntSushi
approved these changes
Aug 8, 2024
Owner
BurntSushi
left a comment
There was a problem hiding this comment.
This is a good catch. I do believe this is indeed correct! Or at least, more correct than the status quo.
I suppose the fully correct version of this would actually utilize the computed min/max values on month to determine the range of days. Like, if Month has a value of 7 with a min/max also equal to 7, then returning a Day whose value, min and max are all 7 is correct. But I suspect that sort of thing doesn't happen in practice. And this patch is stricter than that, so if things are fine with these ranges then that should be okay overall even if it isn't as precise as possible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed that the
days_in_monthfunction returns constant rangeints where the range of the value is just the exact value, so for examplev: 31, min: 31, max: 31. If the idea of the tracked min and max values (in debug builds) is to ensure that the code will work with all possible values of the function in question, then this means we don't actually check the computation for all possible responses todays_in_month.By changing the return values to always have a range with min 28 and max 31 we can ensure that all calculations will work with every month and year, instead of the specific one they are tested with.
A sidenote is that if one returns just a common
Dayvalue fromdays_in_month, which has min of 1 and max of 31, this will end up throwing an error from thenth_weekday_in_monthtests, as the algorithms there only stay in the correct range if the values returned bydays_in_monthare actually what they are commonly.Not sure if I've figured out the situation correctly here – so feel free to just close this pull if it's a mistake.
And to be clear to anybody reading this – the changes here only affect debug builds and the internal safety checks in
jiff. There are no external effects to these changes.