-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove meaningless comment decoration from code #63347
Remove meaningless comment decoration from code #63347
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
r? @Centril |
They are not necessarily meaningless, they may partition code into some logical parts, and highlight those parts and comments applying to those whole parts visually. It doesn't necessarily have to be |
They're not treated as doc comments are they? Some of the ones I left in were trying to illustrate it such as the ones in |
Further to this, if code within a file is in so many distinct parts that it needs partitioning, perhaps it shouldn't be in the same module? |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hmm, I'm not really sure what's causing the errors here. The only diff from master for the file with problems is comment removal. |
☔ The latest upstream changes (presumably #60547) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I'm conflicted about removing some of these section comments; they seem useful and I use them myself from time to time (with // =====
). In other words, I basically agree with @petrochenkov. That said, perhaps the better thing to do here is to look for opportunities to split modules. I would look for the largest sections which fit as a coherent unit and split those out.
No, not at all, the sections may be small and contain just several logically related methods. |
Please, keep the highlighting in |
Personally I think it largely depends on size; if a section is > 500 LOC and makes sense as a logical unit then a module is probably a good thing. |
I've put back the ones in |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
…om-src/libcore/hash, r=Centril Remove unneeded comment in src/libcore/hash/mod.rs Split out from larger PR rust-lang#63347 - other sections in there require further discussion. r? @Centril
…om-src/libcore/hash, r=Centril Remove unneeded comment in src/libcore/hash/mod.rs Split out from larger PR rust-lang#63347 - other sections in there require further discussion. r? @Centril
…om-src/libcore/hash, r=Centril Remove unneeded comment in src/libcore/hash/mod.rs Split out from larger PR rust-lang#63347 - other sections in there require further discussion. r? @Centril
…om-src/libcore/hash, r=Centril Remove unneeded comment in src/libcore/hash/mod.rs Split out from larger PR rust-lang#63347 - other sections in there require further discussion. r? @Centril
…om-src/libcore/hash, r=Centril Remove unneeded comment in src/libcore/hash/mod.rs Split out from larger PR rust-lang#63347 - other sections in there require further discussion. r? @Centril
☔ The latest upstream changes (presumably #63544) made this pull request unmergeable. Please resolve the merge conflicts. |
@Centril I've reviewed each file in this and tried to maintain decoration where the comments don't already stand out sufficiently in the file. I've also removed the decoration where unneeded in comments in the format // ----- comment ------------- or the following format // comment 1 ----------------
// comment two -------------- Does anyone have any strong opinions on any of these files? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in src/test
look good aside from the comments below. Can you extract those to a separate PR including with the comments I've left?
As for the other changes in the code, I continue to feel conflicted about doing the changes there.
It's not clear to me that it is a win and perhaps it's best to just not rock the boat?
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the desire to clean things up. That said, personally, I'm inclined to leave the section headers. Some of them may be superfluous but overall I find them helpful for giving some "structure" to the files. Perhaps that's because I added a lot of them, though. =)
I guess overall it's not something I have a super strong opinion about, but it seems like all of us reviewers are kind of "slightly negative", which makes me think it's not worth the trouble to make the change.
@@ -49,10 +49,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
|
|||
let ptr_size = self.pointer_size(); | |||
let ptr_align = self.tcx.data_layout.pointer_align.abi; | |||
// ///////////////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely want these, I think, as the whole point of this comment is to stand out
@@ -97,10 +97,8 @@ pub fn get_vtable<'tcx, Cx: CodegenMethods<'tcx>>( | |||
}); | |||
|
|||
let layout = cx.layout_of(ty); | |||
// ///////////////////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely want these, I think, as the whole point of this comment is to stand out
Thanks @nikomatsakis. With the ones that've been extracted into other PRs as suggested by Centril a fair amount of the superfluous ones have been removed so I'm happy for this to be closed if you think that's enough. |
@sd234678 thanks for understanding. Closing. |
Closes #62601