Skip to content

Fix a few parser style issues. #2442

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
Aug 6, 2018
Merged

Fix a few parser style issues. #2442

merged 1 commit into from
Aug 6, 2018

Conversation

@zherczeg
Copy link
Member

@zherczeg zherczeg commented Jul 31, 2018

No description provided.

LaszloLango
LaszloLango approved these changes Jul 31, 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

rerobika
rerobika approved these changes Jul 31, 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.

LGTM (informally)

@zherczeg
Copy link
Member Author

zherczeg commented Aug 1, 2018

The issue is that lexer_hex_to_character is used by some internal tests. I don't really like this, but we can keep it if needed.

@zherczeg
Copy link
Member Author

zherczeg commented Aug 3, 2018

Any suggestion?

@LaszloLango
Copy link
Contributor

LaszloLango commented Aug 3, 2018

Let's keep it or declare as an extern function in the related unit test file.

@akosthekiss
Copy link
Member

akosthekiss commented Aug 3, 2018

My two cents: keep lexer_hex_to_character as non-static. There are quite some other non-API functions that are accessed from unit tests, too. (Even in test-lit-char-helpers.c, e.g., lit_char_get_utf8_length.) For now, we have to live with keeping them non-static, IMHO.

Some (distant?) future work may deal with solving this, i.e., how to make those functions static that are only published for testing reasons. But that will deal with all such functions then, not only this one.

@zherczeg
Fix a few parser style issues.
ae57d20 
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
@zherczeg zherczeg force-pushed the parser_style_fixes branch from c9df788 to ae57d20 Compare August 6, 2018 06:16
akosthekiss
akosthekiss approved these changes Aug 6, 2018
View reviewed changes
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

@akosthekiss akosthekiss added the style Related to coding style label Aug 6, 2018
@LaszloLango LaszloLango merged commit 1ac2903 into jerryscript-project:master Aug 6, 2018
@zherczeg zherczeg deleted the parser_style_fixes branch August 10, 2018 06:48
@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

@akosthekiss akosthekiss akosthekiss approved these changes

@LaszloLango LaszloLango LaszloLango approved these changes

@rerobika rerobika rerobika approved these changes

Assignees

No one assigned

Labels

style Related to coding style

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zherczeg @LaszloLango @akosthekiss @rerobika