Move some ast_util functions into methods #30105
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
There's been some discussion about the stability of libsyntax recently, so I'm going to move this over in review: r? @nrc |
src/libsyntax/ast.rs
Outdated
There was a problem hiding this comment.
This should be on the unspanned version. s.node is a Stmt_
|
Good start! However, all your impls should take |
|
Fixed. Are these considered breaking changes? |
|
It's a breaking change for a public, but unstable API. One that isn't used much outside of rustc |
|
@nrc Should this be merged? |
|
☔ The latest upstream changes (presumably #29850) made this pull request unmergeable. Please resolve the merge conflicts. |
src/llvm
Outdated
|
Could you squash the commits please and add a This seems somewhat borderline as to whether it should land or not. I think it is OK - ast_util stuff shouldn't be used by too many syntax extensions. Note that the corresponding changes to the HIR versions are totally OK. There's a bunch of minor things to be addressed, but r+ with them fixed. |
|
Oh, and with tests passing :-) |
|
Will do, thanks very much for the feedback. |
faineance
force-pushed
the
master
branch
2 times, most recently
from
536b2a7 to
0ca7bc5
Compare
faineance
force-pushed
the
master
branch
2 times, most recently
from
46461af to
3fc0dba
Compare
|
Good to go? |
|
@bors: r+ |
|
📌 Commit ec8ea22 has been approved by |
|
⌛ Testing commit ec8ea22 with merge 8f031bf... |
Issue: #30058 Updated for: - Stmt - BinOp_ - UnOp - UintTy, IntTy and FloatTy - Lit - Generics A possible inconsistancy? The `Stmt` methods are on the spanned varient: ```rust pub type Stmt = Spanned<Stmt_>; impl Stmt { pub fn id(s: &Stmt) -> Option<NodeId> { match s.node { StmtDecl(_, id) => Some(id), StmtExpr(_, id) => Some(id), StmtSemi(_, id) => Some(id), StmtMac(..) => None, } } } ``` Whilst the methods for BinOp are on the non spanned version. ````rust impl BinOp_ { pub fn to_string(op: BinOp_) -> &'static str { ... } pub fn lazy(b: BinOp_) -> bool { ... } pub fn is_shift(b: BinOp_) -> bool { ... } pub fn is_comparison(b: BinOp_) -> bool { ... } /// Returns `true` if the binary operator takes its arguments by value pub fn is_by_value(b: BinOp_) -> bool { ... } } pub type BinOp = Spanned<BinOp_>; ```` r? @Manishearth
6 participants
Issue: #30058
Updated for:
A possible inconsistancy?
The
Stmtmethods are on the spanned varient:Whilst the methods for BinOp are on the non spanned version.
r? @Manishearth