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

Issue 4079 #4080

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Issue 4079 #4080

merged 2 commits into from
Mar 31, 2020

Conversation

ahl
Copy link
Contributor

@ahl ahl commented Mar 11, 2020

fixes #4079

Without this fix:

Mismatch at tests/source/issue-4079.rs:3:
 /*!
  * Lorem ipsum dolor sit amet, consectetur adipiscing elit. In lacinia
  * ullamcorper lorem, non hendrerit enim convallis ut. Curabitur id sem
- * volutpat
+  * volutpat
  */
 
 /*! Lorem ipsum dolor sit amet, consectetur adipiscing elit. In lacinia

Mismatch at tests/source/issue-4079.rs:10:
- * ullamcorper lorem, non hendrerit enim convallis ut. Curabitur id sem
- * volutpat */
+  * ullamcorper lorem, non hendrerit enim convallis ut. Curabitur id sem
+  * volutpat */

@@ -528,7 +528,6 @@ impl<'a> CommentRewrite<'a> {
.checked_sub(closer.len() + opener.len())
.unwrap_or(1);
let indent_str = shape.indent.to_string_with_newline(config).to_string();
let fmt_indent = shape.indent + (opener.len() - line_start.len());
Copy link
Member

Choose a reason for hiding this comment

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

I have this nagging suspicion that the offset calc here was used to handle some edge case, though for the life of me I can't think of what that may be nor have I been able to come up with any kind of input that breaks because of this change 🤷‍♂️

As such I think this is probably a good fix, though I wouldn't be completely shocked to see an issue reported at some point in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes agreed that there's some risk here to extant code that lacks corresponding tests. I think this logic may have had to do with some confusion in the code about whether * or ** was the right line continuation on doc block comments.

I looked around for C-style comments in a bunch of rust code. I found very little, but didn't see issues with the code I did find.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@topecongiro topecongiro merged commit a0eb97b into rust-lang:master Mar 31, 2020
@ahl
Copy link
Contributor Author

ahl commented Mar 31, 2020

That's great, thanks! Can I get someone to look at #4071 which is in a similar vein?

@davepacheco
Copy link

Thanks again, @topecongiro! Any idea when this will be released? I was surprised not to find it in the latest nightly I picked up (1.4.14) but I'm not sure how the release process works for rustfmt.

@calebcartwright
Copy link
Member

but I'm not sure how the release process works for rustfmt

@davepacheco - at the moment we're primarily focused on getting rustfmt 2.0 released, so the master branch contains active development for the 2.0 release.

New 1.x versions of rustfmt (like 1.4.14) are only being released to address major bugs or issues with rustfmt being broken on nightly, and typically only contain the minimal set of changes needed to resolve the issue which is why this change wasn't in 1.4.14.

@ahl
Copy link
Contributor Author

ahl commented Apr 18, 2020

@calebcartwright we (@davepacheco and I) found this in our use of rustfmt and wanted to unblock ourselves by submitting the PR. We appreciate you folks accepting the PRs (this and #4071) and any acceleration into a released version would be greatly appreciated. Further, if there are ways we can help with 2.0 we'd be happy to.

@calebcartwright
Copy link
Member

Understood @ahl, and thanks for the PRs and willingness to contribute!

It's up to @topecongiro's when a new release is cut, but I just wanted to provide some background on why this change wasn't in 1.4.14.

If we have to release another 1.x version we can include this change, I just don't know if/when there may be another 1.x version.

@davepacheco
Copy link

Thanks for the context. That's very helpful.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 31, 2022

seems like the backport was completed by #4253

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.

rustfmt mangled C-style inner doc comment
6 participants