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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31601.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Not sure about the first commit. There is no issue or risk with the existing code that is fixed by the style fixups, so to me it seems better to not change it, unless there is a compelling reason. Generally when it comes to style modernizations, my recommendation would be to include them in changes that touch the line anyway, or leave them as-is, or do them wholesale for the whole project (only if there is a good reason for them). But going out and doing them one-by one for specific subsections of the code, especially if the rationale for the change is "consistency" may not be a net benefit.

Also, for the second commit, it may be helpful to add the commit since the change is possible, like I did in f23d6a5, where I presume it is cherry-picked from?

I haven't seen your commit @maflcko, I've recreated it - do you want me to cherry-pick that here instead?

It is already part of https://github.com/bitcoin/bitcoin/pull/31519/commits and I am not sure if it is relevant enough to be split up

If other reviewers think it's relevant enough we can simplify that draft PR of yours to focus on Spans.

Not sure about the first commit. There is no issue or risk with the existing code that is fixed by the style fixups, so to me it seems better to not change it, unless there is a compelling reason. Generally when it comes to style modernizations, my recommendation would be to include them in changes that touch the line anyway, or leave them as-is, or do them wholesale for the whole project (only if there is a good reason for them). But going out and doing them one-by one for specific subsections of the code, especially if the rationale for the change is "consistency" may not be a net benefit.

Agree with this. I don't have an issue with any of the individual changes in the first commit, but most of them would apply all over the codebase. And doing that all over would be a recipe for never-ending churn.

So unless the compiled output for the first commit looks any different, I'd rather not be doing random opinionated changes like this.

And doing that all over would be a recipe for never-ending churn

Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

I'd rather not be doing random opinionated changes like this.

Previously, I've applied the following changes (as mentioned in the commit message), but I agree that a few of these could be reverted as being indeed opinionated:

• Replaced unsigned char in ByteType concept with uint8_t for consistency.
• Used a single ByteType auto* parameter to reduce template boilerplate, as suggested by Russ.
• Replaced hardcoded memcpy lengths (2,4,8) with sizeof(x) and switched to std::memcpy.
• Added noexcept to functions.
• Used brace initialization instead of = for consistency.
• Marked values in write methods as const to make explicit what is being written.

Done in https://github.com/bitcoin/bitcoin/compare/9908ab0581ec7a873514a09edb27a7cbaba3611d..bf5baae88e350365cc07d1fbc38b47ce7f701e13

And doing that all over would be a recipe for never-ending churn

Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

uint8_t and unsigned char are the exact same type. If not, the compilation would fail in other places. I don't think it is helpful to create 2432 pull requests to change each instance one-by-one in a separate pull request. If this change is worth it, it would be better to just do it once for all code and enforce it with a check. If the change isn't worth it, then it would be better to not do it.

And doing that all over would be a recipe for never-ending churn

Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

Not change for change's sake, no. Please consider this from the POV of reviewers and maintainers. A constant stream of changes like that is exhausting. It would distract from larger improvements.

I think @maflcko's comment above is a good description of the culture of this project: modernize the code when you touch it, and if it's important enough to apply to the rest of the codebase, do it in one shot and be done with it.

We (and bots) could fill endless PRs with things that make things a tiny bit more modern/maintainable/consistent, but we don't for the sake of the sanity of our peers.

This PR was a follow-up from the comments on Marco's change.
The main changes are Russ's template simplification and my suggestion to const the writes to obviate which param we're modifying.

const

uint8_t is just an example. It can be applied to const as well. (There is an argument to apply const to references where it makes sense, because it is part of the interface there and "spreads". Though for objects, it is mostly style). Historically, I don't think a change has ever been done and merged to add const to an object. Also, the style guide doesn't even mention it (one way or the other).

Though for objects, it is mostly style

It's not purely a stylistic change - it clarifies which parameter remains unchanged (i.e., it isn’t a "return value").

It seems there’s a strong preference against this change, so I don't mind adjusting the approach or closing the PR altogether.

If there is a good reason to use const for an object (like it being exposed in a larger scope, or it preventing a bug that would otherwise be uncaught), it can be used. However, the change here affects a scope that encompasses at most the next line (or the line after the next line), so it couldn't be smaller. Also, it is hard to see what kind of bug this would be preventing. Either the line of code is correct (with respect to const), or if it wasn't, the existing tests wouldn't be passing anyway.

I'm not sure these arguments would hold in other cases (i.e. "no compiler safety needed since the tests would fail otherwise"), but I've removed the consts as well, basically only leaving @ryanofsky's changes.

The example holds also for the template: Externally they are exactly the same, so they could at most affect the function body, which is 2 or 3 lines. I just don't see what type of issue the change is trying to prevent, or what benefit it brings to developers or users.

(For reference, the commit that drops the unused casts (f23d6a5) has already been merged.)

(For reference, the commit that drops the unused casts (f23d6a5) has already been merged.)

Actual commit: fad4032