Improve whitespace in bswap_64 implementation to make it more readable and easily verifiable.

Thanks for this @danra , but I don't think whitespace-only PRs are required (they increase code-churn, take reviewer time and may cause rebases for other PRs), so I'd rather NACK this.

Tend to agree with @jnewbery. This is also mentioned in the developer notes.

Closing this for now, unless someone has a strong opinion to the contrary.

@MarcoFalke Do you mean "Do not submit patches solely to modify the style of existing code." from the developer notes?

While this change is trivial, its intention isn't to modify the style - it's to make the code more easily verifiable and therefore secure, e.g., there is no zero missing or an extra f stuck somewhere in the masks.

Yes, that is what I was referring to. Generally, it is preferred that style issues are addressed in pulls that touch the code for other reasons than formatting and style.

Even though I understand your motivation, your change needs to be verified as well. This might be simple, but the overhead of a pull request over its whole life span is not negligible.

@MarcoFalke "Generally, it is preferred that style issues are addressed in pulls that touch the code for other reasons than formatting and style."

To me, this seems to contrast the popular convention of atomic commits.

I believe the cost of the overhead of a pull request is much smaller than the cost of harder to review commits, combined with the cost of harder-to-follow history of changes, and harder reverts and merges, whenever needed.

I think you underestimate the overhead of a pull request for our project, which has much stricter requirements than some random mobile app.

Overhead of a pull request

Any proposed change to Bitcoin Core has to go through the review and merge pipeline on GitHub. Bitcoin Core is a probably the most common fully validating node software out there, so any change to the software needs to be carefully reviewed and tested, regardless how "trivial" the change is.

This gives a quick overview of the overhead involved when going through the whole merge pipeline.

Notification overhead

Opening a pull request or sending a comment on any thread on GitHub will send an email to all people that subscribed to Bitcoin Core activity over GitHub, probably several hundred people. Thus, it makes sense to combine related commits into a single commit or pull request to minimize the notifications overhead. Though, you should still keep the patch set at a reasonable size such that review can be done in one sitting.

Review overhead

Any patch needs to be closely reviewed by at least one regular contributor. Even for one line changes, the reviewer needs to recall the whole call graph to check that all edge cases are handled properly. Again, it holds that related change sets should be combined but kept at a size that they still fit into a reviewer's memory. Even if knowledge of the call graph is not required for review, a contributor still has to review every changed line.

Archaeological overhead

All commits to Bitcoin Core are tracked by the git version control system. This is extremely useful when digging up the history of a change to determine its motivation. However, when the same lines of code get frequently changed, it gets increasingly difficult to determine the original intention of a patch set and reason about the effects of that patch set on the current code base.

Maintainability

Before a commit can be merged, someone must run all unit and functional tests. The commits need to be merged, and the merge commit to be signed and verified. Even though we have scripts to handle most of this pain, it still requires human attention by one of the maintainers.