-
Notifications
You must be signed in to change notification settings - Fork 683
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 |
* | ||
* @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_time
andjerry_port_get_time_zone
function calls. Moved old code to default port implementation.Removed
ENABLE_DATE_SYS_CALLS
as date syscalls should "just work".Fixed DST adjustments: even if
gettimeofday
returns 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".