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

Formatting pre and post cast comments enhancements #4406

Merged
merged 4 commits into from
Sep 6, 2020

Conversation

davidBar-On
Copy link
Contributor

Enhancements for formatting pre and post cast comments: /* pre */ as /* post */.
The enhancements are based on discussions and issues examples in #4391.
Test cases were added in issueCastComments.rs.

Changes assume that pre/post cast comments should be trimmed, i.e. that comments' original leading and trailing whitespaces should be removed.

Changes seem to fix issue #3528 and issues of leaving trailing spaces which may be related to #2896. Test cases for these issues were also included .

@calebcartwright
Copy link
Member

Thanks for the PR! Just as a heads up It'll probably be a few days before I have the bandwidth to go through this, need to deal with the broken toolstate issue on nightly first.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the PR @davidBar-On, and all the accompanying tests! It looks fairly good overall and seems like the approach will be viable, although we will need to do a bit of refactoring before merging.

I've added inline comments which detail that needed refactoring for your review, please let us know if you have any questions!

src/formatting/expr.rs Outdated Show resolved Hide resolved
@@ -172,8 +184,11 @@ where
RHS: Rewrite,
{
let tab_spaces = context.config.tab_spaces();
let infix_result = format!("{}{}", pp.infix, pp.infix_suffix);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let infix_result = format!("{}{}", pp.infix, pp.infix_suffix);
let infix_result = format!("{}{}", pp.infix, pp.infix_suffix);
let infix_suffix_separator = if pp.infix_suffix.is_empty() { "" } else { " " };
let infix_prefix_separator = if pp.infix_prefix.is_empty() { "" } else { " " };

@@ -182,12 +197,12 @@ where
};
let lhs_result = lhs
.rewrite(context, lhs_shape)
.map(|lhs_str| format!("{}{}", pp.prefix, lhs_str))?;
.map(|lhs_str| format!("{}{}{}", pp.prefix, lhs_str, pp.infix_prefix))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map(|lhs_str| format!("{}{}{}", pp.prefix, lhs_str, pp.infix_prefix))?;
.map(|lhs_str| format!("{}{}{}{}", pp.prefix, lhs_str, infix_prefix_separator, pp.infix_prefix))?;


// Try to put both lhs and rhs on the same line.
let rhs_orig_result = shape
.offset_left(last_line_width(&lhs_result) + pp.infix.len())
.and_then(|s| s.sub_width(pp.suffix.len()))
.and_then(|s| s.sub_width(pp.suffix.len() + pp.infix_suffix.len()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.and_then(|s| s.sub_width(pp.suffix.len() + pp.infix_suffix.len()))
.and_then(|s| s.sub_width(pp.suffix.len() + pp.infix_suffix.len() + infix_suffix_separator.len()))

src/formatting/pairs.rs Outdated Show resolved Hide resolved
src/formatting/pairs.rs Outdated Show resolved Hide resolved
src/formatting/pairs.rs Outdated Show resolved Hide resolved
src/formatting/pairs.rs Show resolved Hide resolved
tests/source/issue-CastComments.rs Outdated Show resolved Hide resolved
tests/source/issue-CastComments.rs Outdated Show resolved Hide resolved
@davidBar-On
Copy link
Contributor Author

Inline suggestions below for moving the infix suffix/prefix spacing logic into this function as mentioned in the other inline comment in expr.rs. Written somewhat hastily inline in the GitHub Web UI (always dangerous 😆) so may not be perfect, but should be enough to express an idea how to implement the needed change

I have no comments to the suggested changes (look quite final to me ...). I will update and test the PR accordingly.

@calebcartwright
Copy link
Member

Thanks for the updates, LGTM!

@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2020

Heads up: filed a regression that bisects to this PR. Would love if one of you could have a look. #4534

  fn main() {
-     let z = (x as f64 / y as f64).floor() as usize;
+     let z = (x as f64 / y as f64).floor() as f64 / y as f64).floor()
+         as f64 / y as f64).floor() as usize;
  }

@ytmimi ytmimi mentioned this pull request Apr 3, 2022
@ytmimi ytmimi added 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release and removed backport-triage labels Apr 3, 2022
davidBar-On added a commit to davidBar-On/rustfmt that referenced this pull request Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants