Skip to content
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

Merged
merged 1 commit into from
Oct 28, 2017
Merged

Lifted generics to account for changes in rust-lang/rust#44766 #2043

merged 1 commit into from
Oct 28, 2017

Conversation

sunjay
Copy link
Member

@sunjay sunjay commented Oct 8, 2017

Following @alexcrichton's instructions here to fix the breakages caused by that PR.

A maintainer of rls/rustfmt (currently @nrc has done this but others can too!) will not merge the PR. The PR can't be merged because CI will be broken. Instead a new branch will be created, and the PR will be pushed to the branch (the PR is left open)

src/visitor.rs Outdated
@@ -432,6 +433,7 @@ impl<'a> FmtVisitor<'a> {
&item.vis,
body,
),
generics,
Copy link
Member Author

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)

@nrc
Copy link
Member

nrc commented Oct 13, 2017

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.

@sunjay
Copy link
Member Author

sunjay commented Oct 13, 2017

@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. :)

@sunjay
Copy link
Member Author

sunjay commented Oct 15, 2017

@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! 😄

@nrc
Copy link
Member

nrc commented Oct 20, 2017

This is now the upstream branch lift-generics

@DSpeckhals
Copy link

DSpeckhals commented Oct 25, 2017

This might still be causing test failures in rustc:

  --> src/tools/rustfmt/src/utils.rs:40:9
   |
40 |         Visibility::Crate(_) => Cow::from("pub(crate) "),
   |         ^^^^^^^^^^^^^^^^^^^^ expected 2 fields, found 1

@nrc nrc merged commit e2a5c78 into rust-lang:master Oct 28, 2017
@sunjay sunjay deleted the lift_generics branch October 28, 2017 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants