-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Validate string literals #222
Conversation
@@ -411,6 +411,7 @@ Grammar( | |||
"PrefixExpr": (), | |||
"RangeExpr": (), | |||
"BinExpr": (), | |||
"String": (), |
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.
Is there any way we can avoid the name String
here (bikeshed: StringLit, StringExpr etc.)?
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.
Not sure... The token is called STRING
and as far as I know the generator requires that you use the token name in CamelCase. Maybe @matklad knows?
// the newline, and all whitespace at the beginning of the next line are ignored | ||
match self.peek() { | ||
Some('\n') | Some('\r') => { | ||
self.skip_whitespace(); |
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.
Am I right in assuming that this will eat the sequence: "\r\n" for windows?
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.
\n
is whitespace playground link, so it should do.
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.
It does
} | ||
} | ||
|
||
#[cfg(test)] |
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.
Awesome
Is there any way we can void redefining |
// the newline, and all whitespace at the beginning of the next line are ignored | ||
match self.peek() { | ||
Some('\n') | Some('\r') => { | ||
self.skip_whitespace(); |
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.
Should this eat a string of the form
"\
<all whitespace>
more text"
where there is an entirely whitespace line. See that \n
is whitespace playground link.
The current behaviour appears to be that, unless I'm mistaken.
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.
It will indeed eat all the whitespace, including new lines, as does the Rust compiler. See this playground link
bors r+ |
Build succeeded |
Related: #6 (some validators are still missing), fixes #27