Skip to content
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

Merged
merged 1 commit into from
Nov 10, 2018
Merged

Validate string literals #222

merged 1 commit into from
Nov 10, 2018

Conversation

aochagavia
Copy link
Contributor

@aochagavia aochagavia commented Nov 9, 2018

Related: #6 (some validators are still missing), fixes #27

@@ -411,6 +411,7 @@ Grammar(
"PrefixExpr": (),
"RangeExpr": (),
"BinExpr": (),
"String": (),
Copy link
Contributor

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.)?

Copy link
Contributor Author

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();
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does

}
}

#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome

@kjeremy
Copy link
Contributor

kjeremy commented Nov 9, 2018

Is there any way we can void redefining String? Maybe it should be StringLiteral or something?

// the newline, and all whitespace at the beginning of the next line are ignored
match self.peek() {
Some('\n') | Some('\r') => {
self.skip_whitespace();
Copy link
Contributor

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.

Copy link
Contributor Author

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

@aochagavia
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Nov 10, 2018
222: Validate string literals r=aochagavia a=aochagavia

Related: #6 (some validators are still missing), fixes #27

Co-authored-by: Adolfo Ochagavía <github@adolfo.ochagavia.xyz>
@bors
Copy link
Contributor

bors bot commented Nov 10, 2018

Build succeeded

@bors bors bot merged commit 3b4c02c into rust-lang:master Nov 10, 2018
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.

Prototype validators API
3 participants