-
Notifications
You must be signed in to change notification settings - Fork 688
Introduce the Date Port API #1018
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
Conversation
|
LGTM |
jerry-core/jerry-port.h
Outdated
| * | ||
| * @return milliseconds since Unix epoch | ||
| */ | ||
| unsigned long int jerry_port_get_current_time (void); |
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.
milliseconds? not seconds? Because this is an uint32 on 32 bit systems, and that is a short time range.
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.
We have to use milliseconds everywhere, so it is the best, so we don't have to use multiplications and divisions. I think ms is good.
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.
The problem is that an uint32 cannot represent ms since Unix epoch. An uint32 can only represent around 47 days in ms.
Is this tested in a 32 bit machine?
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.
I haven't tested. What about using uint64_t for return type?
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.
Note: I checked and you are right. unsigned long isn't enough for 32bit systems.
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.
uint64_t is ok for me. or double
|
Updated |
|
LGTM. Please squash commits |
Replaced `gettimeofday`-related code with `jerry_port_get_current_time` and `jerry_port_get_time_zone` function calls. Moved old code to default port implementation. Removed `ENABLE_DATE_SYS_CALLS` as date syscalls should "just work". Fix DST adjustments: even if `gettimeofday` returns meaningful data in `tz_dsttime`, the value is just a flag (or, according to some sources, a tri-state value: >0 if DST applies, ==0 if DST does not apply, <0 if unknown). Hitherto, the field was simply added to/subtracted from a time value in milliseconds. To use it approximately correctly, the field's value should be multiplied by "millisecs/hour". JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu
|
Sorry I am just now looking at this. I have a single comment: using a |
|
Even if That being said, I tend to agree now that |
Replaced
gettimeofday-related code withjerry_port_get_current_timeandjerry_port_get_time_zonefunction calls. Moved old code to default port implementation.Removed
ENABLE_DATE_SYS_CALLSas date syscalls should "just work".Fixed DST adjustments: even if
gettimeofdayreturns meaningful data intz_dsttime, the value is just a flag (or, according to some sources, a tri-state value: >0 - DST applies, ==0 - DST does not apply, <0 - unknown). Hitherto, the field was simply added to/subtracted from a time value in milliseconds. To use it approximately correctly, the field's value should be multiplied by "millisecs/hour".