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

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.

Conflicts

No conflicts as of last run.

Concept ACK. I think this is fine to do because:

• C++11 braced init is safer, since it has more compile time checks
• This patch has no conflicts as of now
• bitcoind is not changed by this patch. I checked clang++ -O2 and g++ -O2 on my system.

Instead of linking to a pull request for context, I think OP can be improved to give a clear motivation. I think this patch is by itself worthwhile and should be considered independent of the other pull request.

review ACK 9db89e1

However, I can also see a point about not changing existing code for style reasons, unless it is touched.

I took another look here and it seems the changes are incomplete. For example, there are a lot of occurrences in the RPC layer:

        ret.pushKV("height", (int64_t)stats.nHeight);
        ret.pushKV("bestblock", stats.hashBlock.GetHex());
        ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs);
        ret.pushKV("bogosize", (int64_t)stats.nBogoSize);

If the changes are made complete here, it will probably be a change too large and not worth it, so I'll downgrade my ACK to a -0.

I took another look here and it seems the changes are incomplete. For example, there are a lot of occurrences in the RPC layer:

        ret.pushKV("height", (int64_t)stats.nHeight);
        ret.pushKV("bestblock", stats.hashBlock.GetHex());
        ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs);
        ret.pushKV("bogosize", (int64_t)stats.nBogoSize);

If the changes are made complete here, it will probably be a change too large and not worth it, so I'll downgrade my ACK to a -0.

@MarcoFalke the purpose of this PR is to not be complete :D If it was complete, it would simply be #23810.

The scope of this PR as I understand it (although original title was misleading), was to only apply this to integer literals (I originally said "integer constants").

I could expand the scope to all integer constants, but that would potentially be too breaking (according to what you've said). Let's maybe do that in the future.

@MarcoFalke Did my scope clarification (PR only intended to touch integer literals) clarify your concerns? It seems to me this PR only would cause conflicts in two other PRs, so there being lot's of conflicts isn't a concern, and it shouldn't change the binary.

@MarcoFalke if possible, I'd like to continue moving forward with this. Did you have any concerns I haven't addressed?

I have rebased this PR now that #23438 has been merged.

@ajtowns @achow101, this PR will cause a few minor and easily resolvable conflicts in your PR (#24032, #23304). Do either of you have an objection to this PR being merged, and hence requiring a rebase in your PRs?

24032 will need a rebase after #23508 is merged anyway

@PastaPastaPasta are you still working on this?

I'd love for it to get merged. And would be happy to rebase it. But it seems like most maintainers here don't actually want to review / merge it sadly.

Rebased :) Hopefully we can get a few reviews on this simple PR. I have liked seeing in my continued rebases as we are refactoring places where ints were previously used and are now using dedicated types that actually mean something.

Checked that f2fc03e does not change the binary on my system with my compiler