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

Time function improvements (fix #1912) #2863

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nicowilliams
Copy link
Contributor

@nicowilliams nicowilliams commented Aug 28, 2023

@nicowilliams
Copy link
Contributor Author

image

Gotta normalize tm_zone for tests, or maybe change the tests to normalize it. Probably the latter.

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Aug 28, 2023

For Windows we should try using tzname in f_localtime().

@nicowilliams nicowilliams force-pushed the fix_1912 branch 3 times, most recently from a958727 to 3d3fef0 Compare August 28, 2023 05:25
@nicowilliams
Copy link
Contributor Author

nicowilliams commented Aug 28, 2023

We can also work around the lack of a tm_gmtoff field in some cases by checking the local and gm time and computing the difference, just as we can get the TZ name from tzname.

@nicowilliams
Copy link
Contributor Author

We can also work around the lack of a tm_gmtoff field in some cases by checking the local and gm time and computing the difference, just as we can get the TZ name from tzname.

Er, this is fraught. For example, if I try to use localtime_r() and gmtime_r() then difftime() with a tm value that is in DST, I get the wrong result. I'm inclined not to do this.

@nicowilliams nicowilliams added this to the 1.7 release milestone Aug 28, 2023
@nicowilliams nicowilliams changed the title Broken down time needs tm_{isdst,gmtoff,zone} too (fix #1912) Broken down time needs tm_{isdst,gmtoff,zone} too (fix #1912 #2865) Aug 28, 2023
@nicowilliams nicowilliams changed the title Broken down time needs tm_{isdst,gmtoff,zone} too (fix #1912 #2865) Time function improvements (fix #1912 #2865) Aug 28, 2023
src/builtin.c Outdated Show resolved Hide resolved
@nicowilliams nicowilliams force-pushed the fix_1912 branch 4 times, most recently from ab0b905 to 0daf027 Compare August 29, 2023 05:48
@nicowilliams nicowilliams changed the title Time function improvements (fix #1912 #2865) Time function improvements (fix #1912) Aug 29, 2023
@nicowilliams nicowilliams force-pushed the fix_1912 branch 5 times, most recently from abbb192 to 821b125 Compare August 29, 2023 20:48
@nicowilliams
Copy link
Contributor Author

Now with some boundary tests (which found bugs).

@nicowilliams nicowilliams force-pushed the fix_1912 branch 4 times, most recently from 95aca4e to 880457b Compare August 29, 2023 21:52
tests/jq.test Outdated Show resolved Hide resolved
@nicowilliams
Copy link
Contributor Author

I've left resolved comments to explain some things, mainly the range validation checks for all the fields of broken down time (which were wrong in an earlier version).

@nicowilliams nicowilliams removed this from the 1.7 release milestone Aug 29, 2023
@nicowilliams
Copy link
Contributor Author

Well, I think we should merge this in time for 1.7, but I'm clearing the milestone anyways.

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Aug 29, 2023

Ok, we should allow datetimes before 1970, so I'm making the range 1900..9999, though perhaps on 64-bit systems we can go to -9999..9999?

EDIT: Yes, so let's go to 0001..9999 as the range for years. It works on Linux.

@trantor
Copy link
Contributor

trantor commented Sep 7, 2023

mmm was this not merged before 1.7 ? @nicowilliams

@nicowilliams
Copy link
Contributor Author

mmm was this not merged before 1.7 ? @nicowilliams

@itchyny thought it was too risky for 1.7. I intend to do a new release soon with this and a few other things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong timezone names in strftime/strflocaltime for DST
4 participants