Skip to content

Conversation

@DanielBallaSZTE
Copy link

This patch supports creating a RegExp object through the C API.

JerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu

@LaszloLango LaszloLango added api Related to the public API development Feature implementation labels Sep 27, 2018
@LaszloLango LaszloLango added this to the Release 2.0 milestone Sep 27, 2018
@DanielBallaSZTE DanielBallaSZTE force-pushed the regexp-api branch 4 times, most recently from 5272d9d to f5cb0d0 Compare October 1, 2018 11:04
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.

Please fix the style issues.

@LaszloLango
Copy link
Contributor

I was thinking on a slightly different approach. I'd avoid using the flag's enum. I'd simply create RegExp objects from strings in RegExp literal form, like:

jerry_value_t regexp = jerry_create_regexp("/a*/gim");

What do you guys think?

@zherczeg
Copy link
Member

We would need syntax check then. That is complicated.

@LaszloLango
Copy link
Contributor

@zherczeg it is not that complicated. The code is already there, just needs to be exposed. The only question is which API is better to use, which one is more intuitive.

@zherczeg
Copy link
Member

Code is there? I think it requires a full parser environment to do this.

@LaszloLango
Copy link
Contributor

Code is there? I think it requires a full parser environment to do this.

https://github.com/jerryscript-project/jerryscript/blob/master/jerry-core/parser/js/js-lexer.c#L2002

@zherczeg
Copy link
Member

@LaszloLango First arg: parser_context_t *context_p :) So we would need to call a full parser with another flag to limit its functionality. Btw, if you want a regex this way, a jerry_eval do this for you.

@LaszloLango
Copy link
Contributor

@zherczeg don't stop reading on the first line :) All you need from this function is using only the source code pointer (char *).

@zherczeg
Copy link
Member

Regardless calling jerry_eval with a regex has the same effect, so it does not warrant a separate api function.

@DanielBallaSZTE DanielBallaSZTE force-pushed the regexp-api branch 3 times, most recently from 56e3ce8 to a494dae Compare October 16, 2018 12:16
@DanielBallaSZTE
Copy link
Author

DanielBallaSZTE commented Oct 18, 2018

@akosthekiss I've updated the PR, thank you for your suggestions, it simplified the patch a lot.
UPDATE: working on the travis failure

Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

LGTM

@DanielBallaSZTE
Copy link
Author

DanielBallaSZTE commented Oct 31, 2018

@LaszloLango Do you have any other comments to the patch?

@LaszloLango
Copy link
Contributor

I feel like my question is not really answered, but simply ignored. I am still questioning the form of this new API function. IMHO the literal form is easier to use and it is not difficult to implement.

@zherczeg
Copy link
Member

zherczeg commented Nov 5, 2018

@LaszloLango I would like to see your proposal as a patch (you can copy it into here if it is small). It is hard for me to decide which one looks better. However if your new API is the same as jerry_eval("/a*b/g") I would not support it since it is redundant.

@LaszloLango
Copy link
Contributor

@zherczeg you are right, it would be the same. I can accept your argument. I am good with the current approach.

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

@DanielBallaSZTE DanielBallaSZTE force-pushed the regexp-api branch 2 times, most recently from c914759 to 7387577 Compare November 7, 2018 14:08
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.

Only one minor thing before we merge the PR.

This patch supports creating a RegExp object through the C API.

JerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu
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

@LaszloLango LaszloLango merged commit 704eb45 into jerryscript-project:master Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the public API development Feature implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants