-
Notifications
You must be signed in to change notification settings - Fork 686
Refactoring time-related default port implementations #4513
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
Refactoring time-related default port implementations #4513
Conversation
|
I have to admit that I'm not sure whether existing test cases cover these routines properly. I've partially validated the TZA computation with the C code below: #include <stdio.h>
#include <time.h>
double tza_old(time_t time_utc) {
tzset(); // should be executed only once?
// time in broken-down structure, expressed in local time zone
struct tm tm_local;
localtime_r(&time_utc, &tm_local);
// seconds east of UTC (from GNU-specific structure field)
return (double)tm_local.tm_gmtoff;
}
double tza_new(time_t time_utc) {
// time in broken-down structure, expressed as UTC time
struct tm tm_utc;
gmtime_r(&time_utc, &tm_utc);
// time converted back to seconds since Epoch UTC
tm_utc.tm_isdst = -1; // if not overridden, DST will not be taken into account
time_t time_local = mktime(&tm_utc);
// seconds east of UTC (from simple time differences)
return difftime(time_utc, time_local);
}
int main() {
time_t now_utc = time(NULL); // seconds since Epoch UTC
time_t past_utc = now_utc - 182L * 24 * 60 * 60; // cca. half a year ago
printf("tza-old now: %lf\n", tza_old(now_utc));
printf("tza-old past: %lf\n", tza_old(past_utc));
printf("tza-new now: %lf\n", tza_new(now_utc));
printf("tza-new past: %lf\n", tza_new(past_utc));
return 0;
}Built and ran on Ubuntu 18.04.5 LTS (Linux 4.15.0-130-generic x86_64) as: |
lygstate
left a comment
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
|
@akosthekiss Does this what you want? |
|
@lygstate Thanks. Depends on your actual time zone. Looks like GMT-8: PST? Validates part of the code. But the |
|
Yeap, it's GMT-8 |
If I read the code correctly, the is_utc option didn't considerated on _WIN32 |
|
I have a question, If the OS provide what's the current time zone is, can we use the timezone info directly implement jerry_port_get_local_time_zone_adjustment ? |
rerobika
left a comment
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 after the style fix
jerry-port/default/default-date.c
Outdated
| (void) is_utc; /* unused */ | ||
| return 0.0; | ||
| #endif /* HAVE_TM_GMTOFF */ | ||
| #endif |
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 add the missing comments to preprocessor directives. (here and below)
In the non-Windows code paths: - New approach to compute TZA without the need for GNU-specific `struct tm.tm_gmtoff`. - Always using `usleep` to sleep. (No real need for `nanosleep` as port API has sleep granularity of milliseconds.) - Not checking for "time.h" at build configuration time as that header is mandated by the C standard. - Not checking for "unistd.h" at build configuration time as that header is mandated by the POSIX standard (the default port is targeting POSIX systems -- and Windows). - Fixing some macro guards. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu
1e6d6cf to
609a255
Compare
|
Added the pre-processor directive comments. PR updated. Thanks for the review. |
zherczeg
left a comment
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
PR jerryscript-project#4513 caused a measurable slowdown by removing the GNU specific TZA caclutation. This patch reverts this removal but also keeps the general implemtation introduced in jerryscript-project#4513 JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik robert.fancsik@h-lab.eu
PR jerryscript-project#4513 caused a measurable slowdown by removing the GNU specific TZA caclutation. This patch reverts this removal but also keeps the general implemtation introduced in jerryscript-project#4513 JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik robert.fancsik@h-lab.eu
PR jerryscript-project#4513 caused a measurable slowdown by removing the GNU specific TZA calculation. This patch reverts this removal but also keeps the general implementation introduced in jerryscript-project#4513 JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik robert.fancsik@h-lab.eu
PR jerryscript-project#4513 caused a measurable slowdown by removing the GNU specific TZA calculation. This patch reverts this removal but also keeps the general implementation introduced in jerryscript-project#4513 JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik robert.fancsik@h-lab.eu
In the non-Windows code paths:
struct tm.tm_gmtoff.usleepto sleep. (No real need fornanosleepasport API has sleep granularity of milliseconds.)
header is mandated by the C standard.
header is mandated by the POSIX standard (the default port is
targeting POSIX systems -- and Windows).
JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu