-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Add numeric separator lexing #995
Conversation
Codecov Report
@@ Coverage Diff @@
## master #995 +/- ##
==========================================
+ Coverage 59.97% 60.04% +0.07%
==========================================
Files 166 166
Lines 10851 10881 +30
==========================================
+ Hits 6508 6534 +26
- Misses 4343 4347 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Check my comments :)
R: Read, | ||
{ | ||
let mut prev_is_underscore = false; | ||
let mut pos = cursor.pos(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the pos
variable, cant we call cursor.pos()
where we need the position?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to have the correct position when the separator is at the end of the number. If we take cursor.pos()
, then the position is after the separator, on not on the separator.
boa/src/syntax/lexer/number.rs
Outdated
cursor.take_while_ascii_pred(&mut buf, &|c: char| c.is_digit(kind.base()))?; | ||
if cursor.peek()? == Some(b'_') { | ||
return Err(Error::syntax( | ||
"numeric separator not allowed after .", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"numeric separator not allowed after .", | |
"numeric separator not allowed after '.'", |
same here.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'd preffer if one of the "parser people" had a look since I still don't fully grasp it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
This Pull Request fixes #994
It changes the following: