Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUpdate syn and bump version #353
Conversation
|
@dtolnay Mind having a quick look ? Especially the |
| impl Parse for Tag { | ||
| fn parse(input: ParseStream) -> Result<Self> { | ||
| input.parse::<Token![<]>()?; | ||
| let closing = input.parse::<Option<Token![/]>>()?; |
This comment has been minimized.
This comment has been minimized.
dtolnay
Oct 17, 2018
I would write this with the type on the left because ![/]>>()?; is over the threshold of reasonable adjacent punctuation.
let closing: Option<Token![/]> = input.parse()?;| fn parse(input: ParseStream) -> Result<Self> { | ||
| input.parse::<Token![<]>()?; | ||
| let closing = input.parse::<Option<Token![/]>>()?; | ||
| let name = if let Ok(i) = input.parse::<syn::Ident>() { |
This comment has been minimized.
This comment has been minimized.
dtolnay
Oct 17, 2018
I would write this as:
let name = match input.call(Ident::parse_any)? {
ref wildcard if wildcard == "_" => None,
other => Some(other),
};| None | ||
| }; | ||
| input.parse::<Token![>]>()?; | ||
| Ok(Tag { | ||
| kind: if closing.is_some() { TagKind::EndTag } else { TagKind::StartTag }, |
This comment has been minimized.
This comment has been minimized.
| )); | ||
| impl Parse for LHS { | ||
| fn parse(input: ParseStream) -> Result<Self> { | ||
| if let Ok(p) = input.fork().parse::<syn::Pat>() { |
This comment has been minimized.
This comment has been minimized.
dtolnay
Oct 17, 2018
I would write this as:
if input.peek(Token![<]) {
let mut tags = Vec::new();
while !input.peek(Token![=>]) {
tags.push(input.parse()?);
}
Ok(LHS::Tags(tags))
} else {
let p: Pat = input.parse()?;
Ok(LHS::Pattern(p))
}| }; | ||
| let lhs = input.parse::<LHS>()?; | ||
| input.parse::<Token![=>]>()?; | ||
| let rhs = if let Ok(expr) = input.fork().parse() { |
This comment has been minimized.
This comment has been minimized.
dtolnay
Oct 17, 2018
This is probably not doing what you intended. This is bleeding over into the next arm and relying on the fact that { /* ... */ } < TAG > => is not a valid expression -- because => is not a valid beginning of expression. You should parse each arm without parsing the LHS of the next arm as part of the previous arm's expression.
| input.parse::<Token![else]>()?; | ||
| RHS::Else | ||
| }; | ||
| input.parse::<Option<Token![,]>>()?; |
This comment has been minimized.
This comment has been minimized.
dtolnay
Oct 17, 2018
This is too lenient and allows commas to be omitted where they should be required. Have the parser require a comma after else and after expressions that are not block, unless in the last arm.
| } | ||
| ) | ||
| )); | ||
| impl Parse for MatchToken { |
This comment has been minimized.
This comment has been minimized.
dtolnay
Oct 17, 2018
Regarding fork -- I don't see anything in the input syntax that would require fork. All of this can be parsed with two tokens of lookahead. Please implement the parsing without using fork to make debugging easier and error messages better.
This comment has been minimized.
This comment has been minimized.
|
@bors-servo r=dtolnay+SimonSapin |
|
|
|
|
Update syn - servo/html5ever#353 - servo/rust-cssparser#229 - servo/webrender#3264 - servo/media#162 - rust-lang/rust-bindgen#1409 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22085) <!-- Reviewable:end -->
Eijebong commentedOct 17, 2018
No description provided.