Skip to content

Implement JSON object #307

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
Closed
wants to merge 1 commit into from
Closed

Implement JSON object #307

wants to merge 1 commit into from

Conversation

zherczeg
Copy link
Member

@zherczeg zherczeg commented Jul 6, 2015

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
JerryScript-DCO-1.0-Signed-off-by: Roland Takacs [email protected]

@zherczeg zherczeg mentioned this pull request Jul 6, 2015
Closed
@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

This PR continues the #168 PR. Anyway, this patch was waiting for a very long time, so we really should land it soon.

@egavrin egavrin added this to the ECMA builtins milestone Jul 6, 2015
@egavrin egavrin added critical Raises security concerns development Feature implementation labels Jul 6, 2015
@ruben-ayrapetyan
Copy link
Contributor

@zherczeg, there are several unsolved comments in the #168 PR.
Maybe, it would be more convenient to resolve / discuss them, if the comments would be moved to this pull request.

@ruben-ayrapetyan ruben-ayrapetyan self-assigned this Jul 6, 2015
@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

I didn't see any unanswered questions. I think all of them were fixed or no need any actions.

With a big change like this, you can always raise new issues, but the patch will never land (it was opened 5 weeks ago). We need this patch to support JSON. Perhaps we could fix the minor stuff later.

@ruben-ayrapetyan
Copy link
Contributor

@zherczeg, I see that list, introduced by the patch, is not removed.
However, there is discussion about this in the #168 pull request that ends with comment "Yes, maybe it's better to use the ecma_collection with an append functionality.".

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

Could we postpone that later?

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

That requires a major rewrite in the engine.

@ruben-ayrapetyan
Copy link
Contributor

Could you, please, provide more details? What parts would be involved in the rewrite?

Update:
By "parts" I mean engine components.

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

I can open an issue for that. What do you think?

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

Core engine parts, which is not related to JSON support.

@ruben-ayrapetyan
Copy link
Contributor

For me, it seems that only ecma_collection should be extended with "append" functionality, and other parts should not be changed. Do I miss something?

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

I don't know since engine core is not our focus. But if append functionality is missing, there is likely a good reason for that. But if it is easy, tell me how to do it.

@ruben-ayrapetyan
Copy link
Contributor

@zherczeg, I see. We can raise two issues about this:

  • for adding "append" functionality to ecma-collection;
  • for replacing the list with ecma-collection.

Is it ok for you?

I'll finish review of the rest part of the pull request soon.

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

Ok, thank you. I will open these issues.

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 6, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-helpers-json.cpp

if (remainder < 10)
{
ch = (lit_utf8_byte_t) (48 + remainder);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use LIT_CHAR_0 here.

@ruben-ayrapetyan
Copy link
Contributor

Ok, thank you. I will open these issues.

@zherczeg, thank you for raising the issues.

I've finished review of this pull request. I think, that after fixing mentioned issues, the patch would be ok to merge.

@zherczeg
Copy link
Member Author

zherczeg commented Jul 6, 2015

Thank you! I updated everything.

@ruben-ayrapetyan
Copy link
Contributor

@zherczeg, thank you!

After fixing #307 (comment), ok to merge.

@zherczeg
Implement JSON built-in object
461a277 
JerryScript-DCO-1.0-Signed-off-by: Roland Takacs [email protected]
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
@zherczeg
Copy link
Member Author

zherczeg commented Jul 7, 2015

Landed the patch in #307. Thank you.

Anything else?

@egavrin
Copy link
Contributor

egavrin commented Jul 7, 2015

Good to me

@ruben-ayrapetyan
Copy link
Contributor

Anything else?

I think, ok to merge.

@egavrin
Copy link
Contributor

egavrin commented Jul 7, 2015

make push

@egavrin egavrin assigned zherczeg and unassigned egavrin Jul 7, 2015
@zherczeg
Copy link
Member Author

zherczeg commented Jul 7, 2015

Landed in d1a5f7f

@zherczeg zherczeg closed this Jul 7, 2015
@zherczeg zherczeg deleted the json_support branch July 7, 2015 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zherczeg zherczeg

Labels
critical Raises security concerns development Feature implementation
Projects
None yet
Milestone
ECMA builtins
Development

Successfully merging this pull request may close these issues.
3 participants
@zherczeg @ruben-ayrapetyan @egavrin