refactor: modernize recent `ByteType` usages and read/write functions

l0rinc commented at 6:24 pm on January 3, 2025: contributor

Follow-up to #31524, applying a few of the review suggestions to the affected code, while enabling more trivial reads/writes to use the new std::byte interface.

DrahtBot commented at 6:24 pm on January 3, 2025: contributor

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.

Type	Reviewers
Stale ACK	laanwj

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

l0rinc renamed this:
~~Modernize recent `ByteType` usages and simplify read/write functions~~
refactor: modernize recent `ByteType` usages and simplify read/write functions
on Jan 3, 2025

DrahtBot added the label Refactoring on Jan 3, 2025

l0rinc renamed this:
~~refactor: modernize recent `ByteType` usages and simplify read/write functions~~
refactor: modernize recent `ByteType` usages and read/write functions
on Jan 3, 2025

heADBURNROSARY approved

laanwj approved

laanwj commented at 9:37 am on January 6, 2025: member

Code review ACK 9908ab0581ec7a873514a09edb27a7cbaba3611d Good to get rid of a few redundant UCharCasts.

maflcko commented at 10:31 am on January 6, 2025: member

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.

maflcko commented at 11:00 am on January 6, 2025: member

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

l0rinc commented at 11:59 am on January 6, 2025: contributor

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

maflcko commented at 9:07 am on January 8, 2025: member

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

l0rinc commented at 9:11 am on January 8, 2025: contributor

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

theuni commented at 5:50 pm on January 8, 2025: member

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.

l0rinc force-pushed on Jan 9, 2025

l0rinc commented at 10:12 am on January 9, 2025: contributor

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

maflcko commented at 4:37 pm on January 9, 2025: member

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.

theuni commented at 5:53 pm on January 9, 2025: member

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.

l0rinc commented at 6:12 pm on January 9, 2025: contributor

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.

maflcko commented at 12:36 pm on January 16, 2025: member

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).

l0rinc commented at 12:49 pm on January 16, 2025: contributor

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.

maflcko commented at 1:11 pm on January 16, 2025: member

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.

Modernize `ByteType` usage and simplify read/write functions

- 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.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

321a47d48d

Use the new std::byte-enabled read/write methods

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

e97519f2b4

l0rinc force-pushed on Jan 16, 2025

l0rinc commented at 3:30 pm on January 16, 2025: contributor

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.

maflcko commented at 3:43 pm on January 16, 2025: member

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 (https://github.com/bitcoin/bitcoin/commit/f23d6a572d62ff640a28718ff0771c3f0f87f365) has already been merged.)

l0rinc closed this on Jan 16, 2025

l0rinc deleted the branch on Jan 16, 2025

hodlinator commented at 11:21 am on January 17, 2025: contributor

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

Actual commit: https://github.com/bitcoin/bitcoin/commit/fad4032b219e58300f8d0a74bbcf38ef26fa89d0

refactor: modernize recent ByteType usages and read/write functions #31601

Code Coverage & Benchmarks

Reviews

refactor: modernize recent `ByteType` usages and read/write functions #31601