Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Restore some comment formatting for BigInteger #15726

Merged
merged 2 commits into from
Feb 2, 2017

Conversation

axelheer
Copy link
Contributor

@axelheer axelheer commented Feb 2, 2017

I just noticed that some comments lost some well intended new-lines and even an important comment got lost somehow. Thus, I did a fancy git revert on those files (okay, rather trivial, but for casual git users like me...).

Why? An important comment got lost (otherwise I wouldn't have bothered). And, I generally add new-lines after "overall" comments, which apply not only to the next line of code.

@axelheer
Copy link
Contributor Author

axelheer commented Feb 2, 2017

cc @stephentoub

@@ -92,6 +95,7 @@ public void Reduce(ref FastReducer reducer)
{
// Executes a modulo operation using an optimized reducer.
// Thus, no need of any switching here, happens in-line.

Copy link
Member

Choose a reason for hiding this comment

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

Why are these blank lines being added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wrote these lines of code I put some extra space below comments with a more "general character". Means: if a comment is just related to the line below, there is no blank line. If a comment is an overall note to better understand the current function, it is an extra block. Same "problem" as when to group some lines of code using new-lines or not.

As I said, they got lost and I did an revert, re-added them. I can remove them, if the change is too peculiar.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Ok. Some of them to me personally seem like they'd be better without the blank line, but that's just a matter of style. What you have is fine. Thanks.

@stephentoub
Copy link
Member

Thanks, @axelheer. The fixed comments and alignment look good. Not sure why all of those blank lines need to be added, though.

/// https://en.wikipedia.org/wiki/Exponentiation_by_squaring
/// </summary>
// Executes different exponentiation algorithms, which are
// basically based on the classic square-and-multiply method.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be "which are based on ..." IMO

@mellinoe
Copy link
Contributor

mellinoe commented Feb 2, 2017

I also think the blank lines might be a bit overkill. We don't have any concrete guidelines about that, so it's just up to taste. In this case I don't think it really makes the code any clearer and does make the diff a little bit noisier than it needs to be. I don't really feel strongly about it, though. The other changes look good to me.

@stephentoub stephentoub merged commit 26a7e2b into dotnet:master Feb 2, 2017
@stephentoub
Copy link
Member

Thanks, @axelheer.

@axelheer axelheer deleted the pedantic-fix branch February 3, 2017 14:07
@karelz karelz modified the milestone: 2.0.0 Feb 4, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Restore some comment formatting for BigInteger

Commit migrated from dotnet/corefx@26a7e2b
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants