Skip to content

Indentation of secont and following lines in multiline comment#4658

Merged
calebcartwright merged 1 commit into
rust-lang:masterfrom
davidBar-On:comment_multiline_indentation
Feb 13, 2021
Merged

Indentation of secont and following lines in multiline comment#4658
calebcartwright merged 1 commit into
rust-lang:masterfrom
davidBar-On:comment_multiline_indentation

Conversation

@davidBar-On

Copy link
Copy Markdown
Contributor

Fix an issue in light_rewrite_comment() when formatting 2nd (and following) lines that start with * in a multiline comment. Currently, the function prefix it with the byte preceding the *, so it will be aligned with the comment opening /*, assuming this character is a space.

The main problem is that if the byte preceding the * in the original code is a hard-tab, indentation is to the next tab place and not just by one space. Another issue is when the * is the first char in the line. In this case, nothing is added before the * to align it with the /*.

This fix makes sure that the * is prefixed by a space character.

@calebcartwright calebcartwright left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this! Couple minor things but this looks to be in good order and ready for merging after those are addressed 👍

Comment thread src/formatting/comment.rs Outdated
Comment on lines +969 to +974
(
blank,
trim_end_unless_two_whitespaces(left_trimmed, is_doc_comment),
)
})
.map(|(b, l)| format!("{}{}", b, l))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch! I don't see a need for another map call though, let's just return the allocated string in the closure in the previous block like was done before

Suggested change
(
blank,
trim_end_unless_two_whitespaces(left_trimmed, is_doc_comment),
)
})
.map(|(b, l)| format!("{}{}", b, l))
format!(
"{}{}",
blank,
trim_end_unless_two_whitespaces(left_trimmed, is_doc_comment),
)
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,46 @@
// Ensure multiline comments are indented properly,
// incluyding when second line is prefixed by tab or at the beginning of the line

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo fix 😄 also, can you add a copy of this test case but with the hard_tabs set to true? I wouldn't expect any issues, but good to have for regression regardless

Suggested change
// incluyding when second line is prefixed by tab or at the beginning of the line
// including when second line is prefixed by tab or at the beginning of the line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed and added ...-hard-tabs.rs test file.

@calebcartwright

Copy link
Copy Markdown
Member

Also, make sure you rebase on master given the recent forced bump of the rustc-ap crates

@davidBar-On davidBar-On force-pushed the comment_multiline_indentation branch from 58149d7 to 0fae9bc Compare February 13, 2021 19:38
@davidBar-On

davidBar-On commented Feb 13, 2021

Copy link
Copy Markdown
Contributor Author

Also, make sure you rebase on master given the recent forced bump of the rustc-ap crates

Done, but I probably didn't do it right, as all/some of the latest master changes are also shown as changes here. What I did is to merge the master changes and then git reset --soft to the commit before my changes. Therefore the commit I created now seem to include also the merged master changes, and they are also shown as changes. I don't know how to fix this, except for closing this PR and opening new one.

UPDATE: hopefully fixed the issue by resting the last commit, merging again with master and redoing the changes. Hopefully it is o.k. now.

@davidBar-On davidBar-On force-pushed the comment_multiline_indentation branch from 0fae9bc to 0145848 Compare February 13, 2021 20:29
@calebcartwright

Copy link
Copy Markdown
Member

UPDATE: hopefully fixed the issue by resting the last commit, merging again with master and redoing the changes. Hopefully it is o.k. now.

Thanks! The diff looks okay now. For future reference, I would encourage adding this repo as another upstream in your local copy of your fork

https://docs.github.com/en/github/using-git/adding-a-remote

git remote add upstream https://github.com/rust-lang/rustfmt.git

That way you can continue working with your fork via the same remote (typically origin), but you can easily pull in updates from the upstream repo here:

git fetch upstream

and then you can rebase whatever local branch you are working on against the target upstream branch:

git checkout master
git rebase upstream/master
# or
git checkout comment_multiline_indentation
git rebase upstream/master

@calebcartwright calebcartwright merged commit a36bcfa into rust-lang:master Feb 13, 2021
@davidBar-On

Copy link
Copy Markdown
Contributor Author

I am willing to back-port this PR, but it is not labeled as "1x-backport:pending ". Is it ok to back-port it, or only if/when it will be labeled as "1x-backport:pending "?

A more general question - there are some other PRs I submitted for R2 that are labeled as "1x-backport:pending", but not as "help-wanted". Is it o.k. that I will back-port these PRs?

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.

3 participants