Skip to content

Fix system call related date builtin functions #873

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

Jump to bottom
Merged
merged 1 commit into from
Feb 18, 2016
Merged

Fix system call related date builtin functions #873

merged 1 commit into from
Feb 18, 2016

Conversation

LaszloLango
Copy link
Contributor

Related issues: #213, #691

  • Fixed 'ecma_date_local_tza' and 'ecma_date_daylight_saving_ta' date builtin helper functions
  • Added syscall of gettimeofday function to get the current system time and timezone.
  • Fixed related regression test files.

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

@LaszloLango LaszloLango added bug Undesired behaviour ecma builtins Related to ECMA built-in routines labels Feb 11, 2016
@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 57a217f to 2723f18 Compare February 11, 2016 12:41
zherczeg
zherczeg reviewed Feb 12, 2016
View reviewed changes
jerry-libc/target/darwin/jerry-libc-target.c
@@ -384,3 +386,12 @@ fwrite (const void *ptr, /**< data to write */
return bytes_written / size;
} /* fwrite */

/**
* This function can get the time as well as a timezone.
*/
Copy link
Member

Choose a reason for hiding this comment

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

return value description?

@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 2723f18 to cd38874 Compare February 12, 2016 11:40
@LaszloLango
Copy link
Contributor Author

@zherczeg, I've updated the PR.

@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from cd38874 to 976707e Compare February 15, 2016 13:04
zherczeg
zherczeg reviewed Feb 17, 2016
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-helpers-date.c
@@ -24,6 +24,8 @@
#include "fdlibm-math.h"
#include "lit-char-helpers.h"

#include <time.h>

#ifndef CONFIG_ECMA_COMPACT_PROFILE_DISABLE_DATE_BUILTIN
Copy link
Member

Choose a reason for hiding this comment

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

I think the <time.h> include should be inside the ifndef. It is not needed otherwise.

@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 976707e to 1057970 Compare February 17, 2016 15:14
@LaszloLango
Copy link
Contributor Author

@zherczeg, I've updated the PR.

zherczeg
zherczeg reviewed Feb 18, 2016
View reviewed changes
jerry-libc/include/time.h
#define JERRY_LIBC_TIME_H

#ifdef __cplusplus
# define EXTERN_C "C"
Copy link
Member

Choose a reason for hiding this comment

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

Is not this will be a symbol clash? I think there is a { } based solution for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked fine for me. Could you be more specific? BTW, this is used in every jerry-libc header.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zherczeg here. It would be better to use the more traditional

#ifdef __cplusplus
extern "C" {
#endif
// header main content here
#ifdef __cplusplus
}
#endif

approach. The issue is that the #define EXTERN_C "C" way is used all over the project. I'd love to see that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change it everywhere and add it in a separate commit, but part of this PR.

@LaszloLango
Copy link
Contributor Author

@akiss77, @zherczeg, I've updated the PR.

@zherczeg
Copy link
Member

LGTM

@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 6ccd84e to 442add4 Compare February 18, 2016 11:54
@akosthekiss
Copy link
Member

It's much better practice to have the #ifdef __cplusplus at the very beginning of the headers, not only where function declarations start. In jerry.h I've even spotted that those guards are used twice. So, I'd really prefer to have the order

  • copyright
  • #ifndef HEADERNAME_H #define HEADERNAME_H
  • includes
  • #ifdef _cplusplus #define extern "C" {
  • all header content (including typedes and all, not only function declarations)
  • #ifdef _cplusplus }
  • #endif // HEADERNAME_H

Something like that. Opinion?

@LaszloLango
Copy link
Contributor Author

@akiss77, the suggested style is convenient for me. The biggest problem here is the vera++ checker. We have to modify the rule and currently not sure how.

@zherczeg, @akiss77, do you mind if I raise an issue for this and do it in a separate PR?

@akosthekiss
Copy link
Member

@LaszloLango OK with me

@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 442add4 to 17766ef Compare February 18, 2016 12:56
akosthekiss
akosthekiss reviewed Feb 18, 2016
View reviewed changes
jerry-core/jerry-port.h

/**
* @}
*/

#endif /* !JERRY_API_H */
#ifdef __cplusplus
} /* */
#endif /* !__cplusplus */
Copy link
Member

Choose a reason for hiding this comment

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

The order seems to be mixed up here. JERRY_PORT_H starts, extern C starts, JERRY_PORT_H ends, extern C ends.
And, lo, wrong closing comment! this is not JERRY_API_H...

@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 17766ef to 415293f Compare February 18, 2016 13:30
@LaszloLango LaszloLango mentioned this pull request Feb 18, 2016
Closed
@LaszloLango
Copy link
Contributor Author

A reverted the last commit, so this PR only contains the code related to the Date builtin, because the change of the EXTERN_C macro is not trivial at the moment. I raised an issue for that: #900 and pushed a new branch to my fork: https://github.com/LaszloLango/jerryscript/tree/refactor-extern-c-define

@zherczeg, @akiss77 please check this PR again. Is this ready for merge?

@akosthekiss
Copy link
Member

LGTM (FWIW)

@LaszloLango
Fix system call related date builtin functions
684ed72 
Related issues: jerryscript-project#213, jerryscript-project#691
 * Fixed 'ecma_date_local_tza' and 'ecma_date_daylight_saving_ta' date builtin helper functions
 * Added syscall of gettimeofday function to get the current system time and timezone.
 * Fixed related regression test files.

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
@LaszloLango LaszloLango force-pushed the add-missing-date-functions-to-jerry-libc branch from 415293f to 684ed72 Compare February 18, 2016 14:05
@LaszloLango LaszloLango merged commit 684ed72 into jerryscript-project:master Feb 18, 2016
@LaszloLango LaszloLango deleted the add-missing-date-functions-to-jerry-libc branch February 19, 2016 15:04
@zherczeg zherczeg mentioned this pull request Apr 7, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Undesired behaviour ecma builtins Related to ECMA built-in routines
Projects
None yet
Milestone
Maintenance of ECMA functionality
Development

Successfully merging this pull request may close these issues.
3 participants
@LaszloLango @zherczeg @akosthekiss