-
Notifications
You must be signed in to change notification settings - Fork 898
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
Lifted generics to account for changes in rust-lang/rust#44766 #2043
Conversation
src/visitor.rs
Outdated
@@ -432,6 +433,7 @@ impl<'a> FmtVisitor<'a> { | |||
&item.vis, | |||
body, | |||
), | |||
generics, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you expand the code above, you'll notice that generics is now being passed twice. Once as part of the ItemFn creation, and once as part of visit_fn. This redundancy is further reflected in visit_fn
where the new generics
argument is not actually used in the ItemFn
variant of the match statement. (@nikomatsakis this is what I mentioned to you over chat)
Hey, sorry I haven't got to this for a while, I'm currently on parental leave. The PR looks fine and I can make a branch for this, however, I'm not sure about the parent - should this be rebased on top of current master? There are currently conflicts, which will make updating Rustfmt in the future difficult. |
@nrc Hi, no problem. I hope your parental leave is going well. :) We're currently working on getting this updated to the latest rustfmt master in the PR I linked to. No need to do anything with this until that's fixed. We're blocked on some stuff which is why this hasn't been updated, but that should all be resolved soon. Thanks for taking the time even though you're on leave. :) |
@nrc This is ready for review now. It's now based on the latest master, so it can be merged with no conflicts. As mentioned when I first submitted this PR, please do not merge this yet. I don't think the CI will pass anyway, but I thought I'd mention it anyway. This PR is based on changes that haven't landed in rustc yet, so merging it would break a lot. The merge needs to be done in a certain sequence which @alexcrichton can advise about. Please let me know if you'd like me to make any changes or if you have any questions about the changes I made already. Thanks! 😄 |
This is now the upstream branch lift-generics |
This might still be causing test failures in rustc:
|
Following @alexcrichton's instructions here to fix the breakages caused by that PR.