[Refactor] Apply clang-format to netaddress.h

Fuzzbawls commented at 11:26 am on July 27, 2021: contributor

This brings the entirety of netaddress.h into styling compliance with regard to the indentation of public/private/protected class blocks, per the styling guidelines.

As outlined in the developer documentation, there should be no indentation for public/private/protected class blocks, and this file currently has a “mixed-usage” of indentation vs non-indentation in this regard. This PR serves to bring the file into uniform compliance with the standing guidelines, which should prevent/ease further confusion.

This is a whitespace-only PR, as can be verified by enabling the “Hide whitespace changes” option in GitHub’s WebUI; net 0/0 additions/deletions.

I know blanket refactors such as this are generally discouraged, but seeing as there is little current activity with this file at the moment, and that it is bringing it into styling compliance, I feel it is appropriate/justified if for nothing else than squashing a discrepancy in the existing code styling that is in direct contradiction to the stated guidelines.

Note: I have specifically ignored the pointer/reference side portion of clang-format here specifically to make this a whitespace-only change-set. As such, anyone running the clang-format-diff.py script manually against this commit will see further changes that are intentionally omitted here, but could be added if deemed appropriate.

[Refactor] Apply clang-format to netaddress.h

This brings the entirety of netaddress.h into styling compliance
with regard to the indentation of `public`/`private`/`protected`
class blocks, per the styling guidelines.

366e95b0aa

DrahtBot added the label P2P on Jul 27, 2021

DrahtBot commented at 12:24 pm on July 27, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22375 by MarcoFalke
#22362 by MarcoFalke
#22140 by jonatack

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Fuzzbawls commented at 12:37 pm on July 27, 2021: contributor

Regarding DrathBot’s conflict notices: most, if not all, of them can be resolved trivially as they are whitespace-only conflicts.

fanquake commented at 2:35 am on July 28, 2021: member

Note: I have specifically ignored the pointer/reference side portion of clang-format here specifically to make this a whitespace-only change-set.

Sorry, but this change seems like the worst of both worlds. You’ve opened a change to blanket fix code style, citing a low amount of activity, but at the same time, only fixed a portion of the style?

I was going to suggest at least making the commit a scripted diff, so that it’s more obviously correct, but that can’t be done if this change is hand-crafted.

vasild commented at 7:18 am on July 28, 2021: member

From doc/developer-notes.md:

Do not submit patches solely to modify the style of existing code.

MarcoFalke commented at 7:59 am on July 28, 2021: member

Do not submit patches solely to modify the style of existing code.

I was running into a similar issue recently. While this is probably a good rule for incorrect style in a single line, if a whole block is wrongly indented (e.g. 3 spaces vs 4 spaces), it makes editing that block harder because your editor might re-indent the whole block and you’d have to manually adjust the diff afterward to restore the incorrect indentation. See #22500 (review)

vasild commented at 9:27 am on July 28, 2021: member

I don’t object fixing the style. My eyes are hurt every time I encounter inconsistencies and it is indeed annoying when the editor automatically formats it in another way.

OTOH it is in a clear violation of the guidelines, so maybe just remove that sentence from doc/developer-notes.md or clarify it with what you wrote above.

Ideally I would go for clang-format’ing all source files (scripted diff) at once and making sure that future violations paint the CI red. There is // clang-format off for exceptions. This would make style comments during code review unnecessary. Conflicting PRs can be trivially fixed because it is white-space only changes (like in this PR).

jonatack commented at 9:34 am on July 28, 2021: member

It seems there are many more valuable changes waiting for review, and review is a scarce resource.

Fuzzbawls commented at 9:55 am on July 28, 2021: contributor

I can certainly change this to being done via a scripted-diff to include both the indentation and the pointer/reference alignment inconsistencies in one go for this specific file. I originally didn’t include the pointer/reference alignment change here as that inconsistency is more wide-spread throughout the codebase (and not explicitly mentioned in doc/developer-notes.md, but set as a rule in src/.clang-format), but the indentation inconsistency is isolated to this single file and is explicitly mentioned in `doc/developer-notes.md.

There also seems to be a conflict between the doc/developer-notes.md (as quoted above) and the .github/PULL_REQUEST_TEMPLATE.MD with regard to style-only refactoring submissions: https://github.com/bitcoin/bitcoin/blame/master/.github/PULL_REQUEST_TEMPLATE.md#L28-L35

Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most “code style” refactoring changes require a thorough explanation why they are useful, what downsides they have and why they significantly improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the developer notes, stylistic code changes are usually rejected.

I felt that fixing the indentation inconsistency would improve the developer experience overall by providing consistency. I thought my explanation of the situation was through, and, at least for the indentation inconsistency, it is explicitly mentioned to be preferred in the doc/developer-notes.md file. @vasild in an ideal world, I agree that a single scripted-diff to clang-format the entire source tree would indeed be a “fix to fix all fixes” type solution, but past experiences lead me to believe that it would undoubtedly result in many many conflicts with other open PRs and only serve to drag out, complicate, or even indefinitely stall the review process. this is why I wanted to make this a narrow-focused PR on the single file that violates the indentation preference.

MarcoFalke commented at 9:57 am on July 28, 2021: member

Ideally I would go for clang-format’ing all source files (scripted diff) at once and making sure that future violations paint the CI red

I tried something like this in #6839, which was closed. I don’t think anything has changed since then about re-formatting the whole code base. Also a red CI for “wrong” format is problematic by itself.

See also my reply template:

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

Time spent on review
Accidental introduction of bugs
(Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more information about refactoring changes and stylistic cleanup, see

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

Let me know if you have any questions.

laanwj commented at 2:55 pm on July 28, 2021: member

IMO, please don’t do this. Only make changes that you need to make, in files you need to change. Don’t go randomly reformatting files. I’m pretty sure we explicitly discourage this.

laanwj closed this on Jul 28, 2021

Fuzzbawls commented at 3:40 pm on July 28, 2021: contributor

IMO, please don’t do this. Only make changes that you need to make, in files you need to change. Don’t go randomly reformatting files. I’m pretty sure we explicitly discourage this.

Ok. Might want to update the Pull Request template to reflect this sentiment then, as this was not a “random” reformat, but rather a targeted reformat that I felt fell within the guidelines specifically outlined within that document. Sorry for the inconvenience.

laanwj commented at 3:49 pm on July 28, 2021: member

Ok. Might want to update the Pull Request template to reflect this sentiment then

Sounds good to me. I would refer to doc/developer-notes.md, section “Coding Style (General)”:

Various coding styles have been used during the history of the codebase, and the result is not very consistent. However, we’re now trying to converge to a single style, which is specified below. When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.

Do not submit patches solely to modify the style of existing code.

as this was not a “random” reformat,

Yes, sorry, “random” might be the wrong word. I just mean it comes out of the blue, it’s not a section of the code you’ve been working on.

Sorry for the inconvenience.

No problem, thanks for trying to contribute!

DrahtBot locked this on Aug 18, 2022

[Refactor] Apply clang-format to netaddress.h #22562

Conflicts