Skip to content

Conversation

@Yuyupo
Copy link
Contributor

@Yuyupo Yuyupo commented Jun 5, 2020

JerryScript-DCO-1.0-Signed-off-by: Daniella Barsony [email protected]

JerryScript-DCO-1.0-Signed-off-by: Daniella Barsony [email protected]
@dbatyai dbatyai added the ecma builtins Related to ECMA built-in routines label Jun 5, 2020
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

This custom dispatcher is a good idea!

{
return ecma_builtin_generator_prototype_object_do (executable_object_p,
arguments_list_p[0],
ECMA_ITERATOR_RETURN);
Copy link
Member

Choose a reason for hiding this comment

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

What about putting the type into a local variable and call ecma_builtin_generator_prototype_object_do once after the switch?

arguments_list_p[0],
ECMA_ITERATOR_RETURN);
}
default:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of JERRY_UNREACHABLE, the last case (ECMA_GENERATOR_PROTOTYPE_ROUTINE_RETURN) can be a default case, and put an assert which checks the type.

Copy link
Member

Choose a reason for hiding this comment

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

I'd still insist to the JERRY_UNREACHABLE solution.

Copy link
Member

Choose a reason for hiding this comment

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

Why? It should be one less check to the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I'm adding custom dispatchers is not just for the binary gain but so the code itself is more readable and uniform.
Imho using the default case with an assert is less "pretty" as simply using the switch cases. I would prefer it the way it is now too.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@rerobika rerobika merged commit eb8e81d into jerryscript-project:master Jun 17, 2020
@Yuyupo Yuyupo deleted the generatorDispatcher branch July 8, 2020 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ecma builtins Related to ECMA built-in routines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants