Skip to content

locale.c: use type-agnostic code for storing tm_zone #21949

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 8, 2024
Merged

locale.c: use type-agnostic code for storing tm_zone #21949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions locale.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8044,11 +8044,8 @@ S_ints_to_tm(pTHX_ struct tm * mytm,

/* But since mini_mktime() doesn't handle these fields, save the
* results we already have for them */
# ifdef HAS_TM_TM_GMTOFF
long int gmtoff = mytm->tm_gmtoff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this general enough that we don't have to use your struct tm method for this field too? When writing this, I was surprised that this was a long, since I can't imagine it ever exceeding 14 * 60 * 60 (for double daylight time 12 timezones away from gmt).

Copy link
Contributor Author

@mauke mauke Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14 * 60 * 60 = 50400, which exceeds the range of a 16-bit signed integer type (which is all C requires from int). To get a type that is guaranteed to be at least 32 bits wide, you need long int (or int_least32_t, but that's only C99, I believe).

I removed the gmtoff local variable both for symmetry with zone and because I'd already "paid" for an entire struct tm on the stack, so why not make use of its fields?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somehow I misread this until I downloaded it and looked on my box. You already had done it the way I was suggesting.

# endif
# ifdef HAS_TM_TM_ZONE
const char *zone = mytm->tm_zone;
# if defined(HAS_TM_TM_GMTOFF) || defined(HAS_TM_TM_ZONE)
struct tm mytm2 = *mytm;
# endif

/* Repeat the entire process with mini_mktime */
Expand All @@ -8066,10 +8063,10 @@ S_ints_to_tm(pTHX_ struct tm * mytm,

/* And use the saved libc values for tm_gmtoff and tm_zone */
# ifdef HAS_TM_TM_GMTOFF
mytm->tm_gmtoff = gmtoff;
mytm->tm_gmtoff = mytm2.tm_gmtoff;
# endif
# ifdef HAS_TM_TM_ZONE
mytm->tm_zone = zone;
mytm->tm_zone = mytm2.tm_zone;
# endif

}
Expand Down