refactor: only use explicit reinterpret/const casts, not implicit

PastaPastaPasta commented at 5:22 am on January 28, 2022: contributor

This PR converts 149 silent reinterpret_cast’s into explicit reinterpret_cast’s This PR converts 33 silent const_cast’s into explicit const_cast’s

This PR is broken out from #23810, as it appears there is little appetite for the scope of change as outlined in that PR.

0A C-style cast is equivalent to try casting in the following order:
1
2    const_cast(...)
3    static_cast(...)
4    const_cast(static_cast(...))
5    reinterpret_cast(...)
6    const_cast(reinterpret_cast(...))
7
8For a more thorough discussion, see "ES.49: If you must use a cast, use a named cast"
9in the C++ Core Guidelines (Stroustrup & Sutter).

The PR resolves the most critical step of removing implicit c-style casts that act as reinterpret_casts or const_casts, additionally, this PR should be considerably less conflicting than it’s predecessor. I at least hope that it is deemed the value will exceed the cost of conflicts.

If there are any specific areas of the codebase that are heavily conflicting compared to others, I’d be happy to drop those sections of the PR for now.

DrahtBot added the label P2P on Jan 28, 2022

DrahtBot added the label Refactoring on Jan 28, 2022

DrahtBot added the label RPC/REST/ZMQ on Jan 28, 2022

DrahtBot added the label Utils/log/libs on Jan 28, 2022

DrahtBot added the label UTXO Db and Indexes on Jan 28, 2022

DrahtBot added the label Validation on Jan 28, 2022

DrahtBot added the label Wallet on Jan 28, 2022

DrahtBot commented at 4:01 pm on January 28, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25081 (tracing: lock contention analysis by martinus)
#24962 (prevector: enforce is_trivially_copyable_v by martinus)
#24925 (refactor: make GetRand a template, remove GetRandInt by PastaPastaPasta)
#24922 (Isolate the storage abstraction layer from the application/serialization layer by TheQuantumPhysicist)
#24827 (net: Fix undefined behavior in socket address handling by Adlai-Holler)
#24676 ([WIP] [kernelheaders 1/n] Cleave LevelDB headers from our header tree by dongcarl)
#24464 (logging: Add severity level to logs by klementtan)
#24378 (refactor: make bind() and listen() mockable/testable by vasild)
#21878 (Make all networking code mockable by vasild)
#18014 (lib: Optimizing siphash implementation by elichai)

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.

PastaPastaPasta commented at 8:58 am on January 31, 2022: contributor

@MarcoFalke, do you have any opinions about this more stripped down PR?

maflcko commented at 9:03 am on January 31, 2022: member

Yeah, it might be better to use spans where possible instead of changing one bloaty code to another slightly better, but still bloaty code

PastaPastaPasta commented at 9:15 am on January 31, 2022: contributor

Yeah, it might be better to use spans where possible instead of changing one bloaty code to another slightly better, but still bloaty code @MarcoFalke I agree, seems like there are a number of potential spanifications that would make sense, and maybe I can work on in the future. Do you think that is a good reason to not move forward with this PR? It seems to me that this PR doesn’t have the costs we discussed in #23810, but does have the benefits

+0 for using verbose casts for pointers, since we usually get those wrong (#23438 (comment), https://github.com/bitcoin/bitcoin/pull/23810/files#r771801088).

will yet again need everyone with an existing patch, who does more significant work, to rebase.

Since this PR only conflicts with two others, (one of which seems quite dead) I think this cost has been minimized.

maflcko commented at 9:21 am on January 31, 2022: member

I don’t think it makes sense to change the same LOC twice. See also #22167 (comment) where I suggested to do char->std::byte, but then decided to go char->Span.

There is more cost to a pull request than conflicts. Another cost is review and the general 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.

jonatack commented at 6:25 pm on January 31, 2022: contributor

I like that these named casts make smelly conversions easier to see, grep for, and more verbose/ugly, to eventually encourage refactoring them away. Given how many there are in the codebase though, it’s easier to do it when the code is being touched for other things, either as a PR author or a reviewer making suggestions, and I’ve seen a few reinterpret casts added this way. Doing it wholesale across the codebase is a hard sell, however, for the reasons mentioned above.

DrahtBot added the label Needs rebase on Feb 4, 2022

PastaPastaPasta force-pushed on Apr 7, 2022

DrahtBot removed the label Needs rebase on Apr 7, 2022

DrahtBot added the label Needs rebase on Apr 19, 2022

PastaPastaPasta force-pushed on Apr 19, 2022

DrahtBot removed the label Needs rebase on Apr 19, 2022

laanwj referenced this in commit 43bb106613 on Apr 21, 2022

DrahtBot added the label Needs rebase on Apr 21, 2022

PastaPastaPasta force-pushed on Apr 21, 2022

DrahtBot removed the label Needs rebase on Apr 21, 2022

DrahtBot added the label Needs rebase on Apr 26, 2022

martinus commented at 11:57 am on April 27, 2022: contributor

I had a look through a few of the casts, and I think quite a bit of them can simply be removed. E.g. all casts to void*, and many others as well. Also, I think it might be beneficial to change the hash API to accept const void*. Then a lot more reinterpret_casts could be removed.

maflcko commented at 12:03 pm on April 27, 2022: member

I think the hash api might be better off taking a span of std::byte

martinus commented at 12:15 pm on April 27, 2022: contributor

I think the hash api might be better off taking a span of std::byte

I just saw there are also helpers like MakeByteSpan and AsBytes which would make this convenient, I agree

refactor: replace all silent C-Style reinterpret_cast's with explicit casts bb0f8d759f

refactor: replace all implicit C-style const/const+reinterpret with explicit casts 96cc421f8d

PastaPastaPasta force-pushed on May 11, 2022

DrahtBot removed the label Needs rebase on May 11, 2022

DrahtBot commented at 8:44 am on May 12, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on May 12, 2022

achow101 commented at 7:00 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

PastaPastaPasta commented at 10:06 pm on October 12, 2022: contributor

I’ve just rebased this PR. I’d love for this to move forward, but it seems that most maintainers here are against it due to the conflicts it’ll cause with other open PRs (even though imo the conflicts will be quite easy for authors to resolve)

martinus commented at 5:54 am on October 13, 2022: contributor

@PastaPastaPasta you could remove all the reinterpret_cast<void*> casts

bitcoin locked this on Oct 13, 2023

refactor: only use explicit reinterpret/const casts, not implicit #24185

Conflicts