Skip to content

Implement the core of the map object. #2447

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
Sep 4, 2018
Merged

Implement the core of the map object. #2447

merged 1 commit into from
Sep 4, 2018

Conversation

zherczeg
Copy link
Member

@zherczeg zherczeg commented Aug 6, 2018

Heavily under construction. set/get/has and gc marking is working with Map. Next step is delete. WeakMap is there as well, but I realized the gc marking for WeakMaps can be too costly.

@zherczeg zherczeg force-pushed the implement_map branch 5 times, most recently from dea3bd1 to e161ed2 Compare August 14, 2018 06:08
@zherczeg zherczeg changed the title [WIP] Implement the core of the map object. Implement the core of the map object. Aug 14, 2018
@zherczeg
Copy link
Member Author

Patch ready for review.

rerobika
rerobika reviewed Aug 14, 2018
View reviewed changes
Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

Nice patch, just a few things I've noticed.

jerry-core/ecma/operations/ecma-map-object.c Outdated
bool is_weak_map) /**< map is weak map */
{
JERRY_UNUSED (key_arg);
JERRY_UNUSED (value_arg);
Copy link
Member

Choose a reason for hiding this comment

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

Both JERRY_UNUSED are unnecessary.

@zherczeg zherczeg force-pushed the implement_map branch 2 times, most recently from 53ba82b to ee0afe5 Compare August 14, 2018 12:40
@akosthekiss akosthekiss added ecma builtins Related to ECMA built-in routines ES2015 Related to ES2015 features labels Aug 17, 2018
@zherczeg zherczeg force-pushed the implement_map branch 2 times, most recently from 6e78d44 to 5d58780 Compare August 31, 2018 07:53
LaszloLango
LaszloLango approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

P.S.: It would be great to add more tests to jerry-test-suite/es2015 too. It can be done in a followup PR.

robertsipka
robertsipka requested changes Sep 4, 2018
View reviewed changes
jerry-core/ecma/base/ecma-gc.c Outdated
@@ -33,10 +33,13 @@

#ifndef CONFIG_DISABLE_ES2015_TYPEDARRAY_BUILTIN
#include "ecma-typedarray-object.h"
#endif
#endif /* CONFIG_DISABLE_ES2015_TYPEDARRAY_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_TYPEDARRAY_BUILTIN */

jerry-core/ecma/base/ecma-gc.c Outdated
#endif /* CONFIG_DISABLE_ES2015_PROMISE_BUILTIN */
#ifndef CONFIG_DISABLE_ES2015_MAP_BUILTIN
#include "ecma-map-object.h"
#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/base/ecma-gc.c Outdated
}
} /* ecma_gc_mark_map_object */

#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/base/ecma-gc.c Outdated
ecma_value_p = ecma_collection_iterator_init (((ecma_promise_object_t *) ext_object_p)->reject_reactions);

while (ecma_value_p != NULL)
#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/base/ecma-gc.c Outdated
ecma_dealloc_extended_object (object_p, sizeof (ecma_map_object_t));
return;
}
#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/base/ecma-globals.h Outdated
* the rest can be ECMA_VALUE_ARRAY_HOLE or any valid value. */
} ecma_map_object_chunk_t;

#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/operations/ecma-map-object.c Outdated
* @}
*/

#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/operations/ecma-map-object.h Outdated
* @}
*/

#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !CONFIG_DISABLE_ES2015_MAP_BUILTIN */

jerry-core/ecma/operations/ecma-map-object.h Outdated

#endif /* CONFIG_DISABLE_ES2015_MAP_BUILTIN */

#endif /* ECMA_MAP_OBJECT_H */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* !ECMA_MAP_OBJECT_H */

jerry-core/ecma/base/ecma-gc.c

if (!ecma_is_value_pointer (item))
{
if (ecma_is_value_object (item))
Copy link
Contributor

@robertsipka robertsipka Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It is just a nit-picking, but I think you could merge these two conditions.

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot because we would enter into the else branch when value is pointer.

@zherczeg
Implement the core of the map object.
a0e67d8 
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
@zherczeg
Copy link
Member Author

zherczeg commented Sep 4, 2018

Thank you for the review, patch is updated.

robertsipka
robertsipka approved these changes Sep 4, 2018
View reviewed changes
Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsipka robertsipka merged commit d3d42f7 into jerryscript-project:master Sep 4, 2018
@zherczeg zherczeg deleted the implement_map branch September 4, 2018 13:24
@zherczeg zherczeg mentioned this pull request Mar 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rerobika rerobika rerobika left review comments

@robertsipka robertsipka robertsipka approved these changes

@LaszloLango LaszloLango LaszloLango approved these changes

Assignees
No one assigned
Labels
ecma builtins Related to ECMA built-in routines ES2015 Related to ES2015 features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zherczeg @robertsipka @LaszloLango @rerobika @akosthekiss