Skip to content

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

Merged
merged 1 commit into from
Apr 20, 2016

Conversation

akosthekiss
Copy link
Member

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".

Fixed 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 - 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".

@akosthekiss akosthekiss added enhancement An improvement jerry-port Related to the port API or the default port implementation labels Apr 19, 2016
@akosthekiss
Copy link
Member Author

Related to the proposal #964 .
Acknowledgement to @franc0is for #969 with which this PR overlaps.

@LaszloLango
Copy link
Contributor

LGTM

*
* @return milliseconds since Unix epoch
*/
unsigned long int jerry_port_get_current_time (void);
Copy link
Member

@zherczeg zherczeg Apr 19, 2016

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.

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member

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

@akosthekiss
Copy link
Member Author

Updated

@zherczeg
Copy link
Member

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
@akosthekiss akosthekiss merged commit b523cf3 into jerryscript-project:master Apr 20, 2016
@akosthekiss akosthekiss deleted the port-date branch April 20, 2016 16:26
@franc0is
Copy link
Contributor

franc0is commented Apr 21, 2016

Sorry I am just now looking at this. I have a single comment: using a uint64_t when we know it will be converted to floating point is a little bit wasteful. Why did you decide against using double @akiss77 ?

@akosthekiss
Copy link
Member Author

Even if jerry_port_get_current_time returns double, a "cast" with DOUBLE_TO_ECMA_NUMBER_T is still needed, since the number representation may be float instead of double. (Of course, if numbers are double, that macro doesn't cost anything.)

That being said, I tend to agree now that double would be/have been better than uint64_t. Fortunately, this would take a very small change only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement jerry-port Related to the port API or the default port implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants