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

Fixed comment dropped between & and type issue #4482

Merged
merged 5 commits into from
Nov 3, 2020

Conversation

whizsid
Copy link
Contributor

@whizsid whizsid commented Oct 20, 2020

Fixes #4245 .

@calebcartwright
Copy link
Member

Thank you for the PR @whizsid and apologies for the delay in review. The end result of the formatting changes look pretty good from the test cases, though I do have a few general thoughts for you on the implementation changes in types.rs

  • Bit of a nit but I think using reference or ref_ for local names would be more logical than using "and" naming since we aren't dealing with a logical/bitwise operator in this context
  • Be sure to avoid duplicative span derivations (e.g. there's two lines that currently derive the same span from the ref token through the end of the type)
  • We should be able to leverage the associated Mutability variant (mt.mutbl) to check whether or not the reference is mutable instead of needing multiple lines that scan the associated span snippets for mut. I think this could be shifted up front to provide a reusable bool (e.g. is_mut)
  • Don't hesitate to leverage closures and/or add new functions to both encapsulate some common steps and to simplify things and improve readability. With the current proposed changes, the body for this match arm has nearly tripled in length, but those additions are almost exclusively within the same original match on the lifetime. My preference would be to refactor this a bit to try to minimize the nesting and extra work being done within (for example let last_span = match *lifetime { ... is going a whole lot more than determining the value of last_span).

This PR is a great start, and I'm hoping we can get your fixes tweaked/restructured a bit to achieve that goal more cleanly and with less duplication.

@whizsid
Copy link
Contributor Author

whizsid commented Oct 27, 2020

@calebcartwright Thank you for the review. I changed my PR to meet your suggestions.

We should be able to leverage the associated Mutability variant (mt.mutbl) to check whether or not the reference is mutable instead of needing multiple lines that scan the associated span snippets for mut. I think this could be shifted up front to provide a reusable bool (e.g. is_mut)

Sorry I didn't used it to get only the mutable status. Currently rustc-ap-rustc_ast crate is not providing the Span of the Mutability. So I am calculating the mut_lo and mut_hi by getting the index of mut keyword. A contributor of rust-lang invited me to discuss this on zulip (rust-lang/rust#78315). But I couldn't discuss it yet.

Github actions are failing because of an issue of the rustc-ap-rustc_data_structures crate.

@calebcartwright
Copy link
Member

Thank you for the updates @whizsid, could you rebase to grab the latest changes off master to make CI happy?

@whizsid
Copy link
Contributor Author

whizsid commented Oct 28, 2020

@calebcartwright CI Failed again because of a network error.

@calebcartwright
Copy link
Member

Thanks for the update @whizsid! I find this to be easier to read 👍

One minor item left inline to leverage ast::Mutability to avoid doing unnecessary work, but otherwise LGTM

@calebcartwright
Copy link
Member

Excellent, thank you for the PR and all the good work you've been doing lately @whizsid!

@calebcartwright calebcartwright merged commit 4abdda1 into rust-lang:master Nov 3, 2020
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this pull request Nov 29, 2020
* Fixed comment dropped between & and type issue

* Reduced nesting levels and avoided duplications

* Removed extra allocations
calebcartwright pushed a commit that referenced this pull request Nov 29, 2020
* Fixed comment dropped between & and type issue

* Reduced nesting levels and avoided duplications

* Removed extra allocations
@karyon
Copy link
Contributor

karyon commented Oct 26, 2021

Backported in #4564

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment dropped between & and typename in parameter type
3 participants