Uh oh!
There was an error while loading. Please reload this page.
[breaking batch] don't glob export ast::UnOp variants - #31487
Conversation
rust-highfive
commented
Feb 8, 2016
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (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. |
Manishearth
commented
Feb 8, 2016
Feel free to just make a single PR which you keep adding commits to to avoid needing a rebase |
da37149 to
1bb12a6Comparebluss
commented
Feb 8, 2016
Fyi the marker is exactly |
a0a5215 to
b177233Compareoli-obk
commented
Feb 10, 2016
ok, this is ready, I'm done for this run. Before the rebase + squash it passed locally. Let's see what travis says. The only potentially controversial changes are in libsyntax/ast.rs, everything else is just fallout due to the renamings or removal of glob exports. |
b177233 to
acf3dc4Compareoli-obk
commented
Feb 10, 2016
jup travis likes it :) |
Manishearth
commented
Feb 11, 2016
Reviewed most of it, have marked places where I'm unsure of something. |
acf3dc4 to
74e6082Compareoli-obk
commented
Feb 11, 2016
addressed comments |
oli-obk
commented
Feb 11, 2016
the PR already bitrotted (see travis). rebasing... |
llogiq
commented
Feb 11, 2016
@petrochenkov I don't think it's too ugly, but I guess that's a matter of taste. |
petrochenkov
commented
Feb 11, 2016
Well, ok. I guess. |
Manishearth
commented
Feb 11, 2016
@petrochenkov In the past we didn't have a concrete policy on breakages. Try to make a PR before this makes the nightly, basically. The window is flexible, though. |
Manishearth
commented
Feb 11, 2016
Also, if someone wants to do the same reexport change to the HIR it would be nice. Don't worry about breakages there, since only clippy and some other lints depend upon the HIR and we can take care of that. |
Manishearth
commented
Feb 11, 2016
btw, regarding this, there wasn't much discussion on this PR, but people affected by breaking changes were pinged a couple of days ago on the older version of this PR (which was split up). Usually I'd wait a few days so that further breaking changes can be collected; however this itself is a large collection of breakages and is large enough that it will bitrot almost every few hours. In the future, if you feel the need to make a breaking change, make a PR, get reviewed (without the r+), and when we have enough of them around we can roll them up. |
cc #31487 plugin-[breaking-change] The AST part of #30087 r? @Manishearth
llogiq
commented
Feb 13, 2016
@oli-obk perhaps you can say something about https://internals.rust-lang.org/t/nightly-broke-quote-tokens-1i32/3174/2 ? |
sgrif
commented
Feb 14, 2016
Is there some reasoning for this change? While I understand that we don't want libsyntax to be defacto stable, this seems like really unnecessary breakage that affects basically every crate doing anything with macros.
Nobody on the Diesel team was pinged. :\ |
rust-lang/rust#31487 included breaking changes which cause codegen to no longer compile on the latest nightly. It will likely be several days before I can synchronize everything to work on both nightly and stable against the latest versions.
This is due to some apparent changes in Rust nightly that is causing the `extern crate` definitions to conflict with each other. Unfortunately, we still don't compile on nightly as a result of rust-lang/rust#31487, but this is an easy win.
Manishearth
commented
Feb 14, 2016
@sgrif Ah, sorry. I didn't know that Diesel uses syntax extensions a lot. The main breakage I was worried about was serde/aster (which is used by half the ecosystem). I'll ping y'all next time this happens, and keep a tracking issue where we can coordinate future libsyntax breaking changes. |
cc #31487 (comment) plugin-[breaking-change] The first commit renames `ast::Pat_` to `ast::PatKind` and uses its variants in enum qualified form. I've also taken the opportunity and renamed `PatKind::Region` into `PatKind::Ref`. The second commit splits `PatKind::Enum` into `PatKind::TupleStruct` and `PatKind::UnitStruct`. So, pattern kinds now correspond to their struct/variant kinds - `Struct`, `TupleStruct` and `UnitStruct`. @nikomatsakis@nrc@arielb1 Are you okay with this naming scheme? An alternative possible naming scheme is `PatKind::StructVariant`, `PatKind::TupleVariant`, `PatKind::UnitVariant` (it's probably closer to the common use, but I like it less). I intend to apply these changes to HIR later, they should not necessarily go in the same nightly with #31487 r? @Manishearth
Adapt to nightly changes introduced by rust-lang/rust#31487
It was pointed out in review that the glob-exported underscore-suffixed convention for `Spanned` HIR nodes is no longer preferred: see February 2016's rust-lang#31487 for AST's migration away from this style towards properly namespaced NodeKind enums. This concerns rust-lang#51968.
r? @Manishearth
I just noticed they can't be rolled up (often modifying the same line(s) in imports). So once I reach the critical amount for them to be merged I'll create a PR that merges all of them.