Upgrade to Syn 2 #1271
Upgrade to Syn 2 #1271
Conversation
There was a problem hiding this comment.
cc @ahomescu since he's working on resurrecting c2rust-refactor. Generally, I think upgrading syn to 2 is good. @fw-immunant do you have any concerns?
|
This is a pretty wide-reaching change that might have impacts all over; I want to give it a close review when I get a chance. |
| else_branch: Some((token, else_)), | ||
| .. | ||
| }), | ||
| _semi, |
There was a problem hiding this comment.
This occurrence still needs fixing.
| impl SetSpan for Block { | ||
| fn set_span(&mut self, s: Span) { | ||
| if self.stmts.is_empty() == false { | ||
| self.stmts[0].set_span(s); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there still a way to set the span for the brace token so that we handle empty blocks?
There was a problem hiding this comment.
self.brace_token.span is DelimSpan, and DelimSpan::new() is pub(crate) so this is the closest functionality to set span. IMHO, if we want to revive comment functionality, i think the best way is to contribute changes in prettyplease & syn to add comment in AST node. The current way to store comment id is already abusing poor span 😅
There was a problem hiding this comment.
Yes, our current span-mangling is really hacky. I'm not sure adding comments to AST nodes in syn would be acceptable upstream nor whether it would be sufficient for perfect comment preservation. That said, it would be good to have a comment here to explain the omission. I can add one in a follow-up; don't consider this a blocker.
|
hi @ fw-immunant, thanks for the review. will check it tomorow |
…Literal in IntLit
…match the existing behavior
| else_branch: Some((token, else_)), | ||
| .. | ||
| }), | ||
| _semi, |
There was a problem hiding this comment.
This occurrence still needs fixing.
| impl SetSpan for Block { | ||
| fn set_span(&mut self, s: Span) { | ||
| if self.stmts.is_empty() == false { | ||
| self.stmts[0].set_span(s); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Yes, our current span-mangling is really hacky. I'm not sure adding comments to AST nodes in syn would be acceptable upstream nor whether it would be sufficient for perfect comment preservation. That said, it would be good to have a comment here to explain the omission. I can add one in a follow-up; don't consider this a blocker.
upgrade to syn 2 for crate
c2rust-transpile,c2rust-ast-builder,c2rust-bitfields-derive, andc2rust-ast-printer.c2rust-macrosexcluded because it only used byc2rust-refactorsyn introduces many breaking changes which can be viewed in https://github.com/dtolnay/syn/releases/tag/2.0.0