Skip to content

Syntax 0.8, part 7: \UHHHHHH and other changes #314

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.

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

Merged
merged 3 commits into from
Nov 30, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Nov 28, 2018

Implement part 7 of #303.

@stasm stasm mentioned this pull request Nov 28, 2018
15 tasks
@stasm stasm changed the base branch from master to zeroeight November 29, 2018 09:04
@stasm
Copy link
Contributor Author

stasm commented Nov 29, 2018

This PR depends on #313 which hasn't been merged yet. @Pike, if you want to review this first, the compare view might be helpful. Thanks.

Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

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

Just a nit about checking u and U twice, otherwise, nice, r=me.

for (let i = 0; i < 4; i++) {
const ch = ps.takeHexDigit();
getUnicodeEscapeSequence(ps, u, digits) {
ps.expectChar(u);
Copy link
Contributor

Choose a reason for hiding this comment

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

We already checked this in the switch statement, I'd just ps.next() here, and drop the u argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to keep expectChar. In fact, I'm planning a clean up after 0.8 where we get rid of most of ps.next(). I always seem to forget what it's supposed to consume, something expectChar is very explicit about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and we need the u arg for the error message. That's the real reason why it's there.

For compatibility with the reference parser which used to use the /./
regex, the tooling parser used to recognize additional EOL characters in
Comments. This has since been fixed in the reference parser:

    projectfluent/fluent@2d224cb
@stasm stasm merged commit 730f005 into projectfluent:zeroeight Nov 30, 2018
@stasm stasm deleted the zeroeight-part7 branch November 30, 2018 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants