Skip to content

Conversation

@rtakacs
Copy link
Contributor

@rtakacs rtakacs commented Nov 20, 2019

No description provided.

const uint8_t *src_end_p = src_p + length - 1;

#if ENABLED (JERRY_ES2015)
if (src_p[1] == LIT_CHAR_LOWERCASE_O)
Copy link
Member

@rerobika rerobika Nov 20, 2019

Choose a reason for hiding this comment

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

This should be LEXER_TO_ASCII_LOWERCASE (src_p[1) == LIT_CHAR_LOWERCASE_O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! You are right :)

@rtakacs rtakacs force-pushed the es6-octal-literal branch 2 times, most recently from 6641aa9 to ea863dd Compare November 20, 2019 17:25
@rerobika rerobika added ES2015 Related to ES2015 features parser Related to the JavaScript parser labels Nov 20, 2019
Copy link
Contributor

@ossy-szeged ossy-szeged left a comment

Choose a reason for hiding this comment

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

It's great to support octal literals. But I think we should make Number(...) too to be able to parse octal numbers too. Of course, it can be done in a follow-up PR.


if (*source_p < context_p->source_end_p
&& *source_p[0] >= LIT_CHAR_8
&& *source_p[0] <= LIT_CHAR_9)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure if this check is correct? Maybe I'm wrong, but I can't see what is it for.

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 part of code checks if an octal literal contains 8 or 9 digits (that is not allowed). (A simple copy paste from the original code.) But it can be simplified :)

// limitations under the License.

assert (0o10 === 8);
assert (0O10 === 0o10);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more tests, maybe invalid tests too to check corner cases of the parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course.

@rtakacs rtakacs force-pushed the es6-octal-literal branch 2 times, most recently from aa9e5af to 5cd750f Compare November 21, 2019 07:35
if (*source_p < context_p->source_end_p
&& (*source_p[0] == LIT_CHAR_8 || *source_p[0] == LIT_CHAR_9))
{
parser_raise_error (context_p, PARSER_ERR_INVALID_NUMBER);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use the PARSER_ERR_INVALID_OCTAL_DIGIT error here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is more precise error message. I've modified the PR.

Copy link
Member

@dbatyai dbatyai left a comment

Choose a reason for hiding this comment

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

LGTM

JerryScript-DCO-1.0-Signed-off-by: Roland Takacs [email protected]
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 996bf76 into jerryscript-project:master Nov 21, 2019
@rtakacs rtakacs deleted the es6-octal-literal branch March 24, 2020 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ES2015 Related to ES2015 features parser Related to the JavaScript parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants