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

PastaPastaPasta · 2022-01-28T05:22:25Z

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.

A C-style cast is equivalent to try casting in the following order:

    const_cast(...)
    static_cast(...)
    const_cast(static_cast(...))
    reinterpret_cast(...)
    const_cast(reinterpret_cast(...))

For a more thorough discussion, see "ES.49: If you must use a cast, use a named cast"
in 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 · 2022-01-28T16:01:07Z

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 · 2022-01-31T08:58:39Z

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

maflcko · 2022-01-31T09:03:51Z

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 · 2022-01-31T09:15:45Z

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 · 2022-01-31T09:21:25Z

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 · 2022-01-31T18:25:05Z

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.

3ae7791 refactor: use Span in random.* (pasta) Pull request description: ~This PR does two things~ 1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes ~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided. This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~ MarcoFalke this was inspired by your comment here: bitcoin/bitcoin#24185 (comment) about using Span, so hopefully I'll be able to get this PR done and merged 😂 ~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~ ACKs for top commit: laanwj: Thank you! Code review re-ACK 3ae7791 Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79

martinus · 2022-04-27T11:57:46Z

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 · 2022-04-27T12:03:00Z

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

martinus · 2022-04-27T12:15:04Z

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

… casts

…xplicit casts

DrahtBot · 2022-05-12T08:44:54Z

🐙 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".}

achow101 · 2022-10-12T19:00:54Z

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.

PastaPastaPasta · 2022-10-12T22:06:27Z

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 · 2022-10-13T05:54:42Z

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

DrahtBot added P2P Refactoring RPC/REST/ZMQ Utils/log/libs UTXO Db and Indexes Validation Wallet labels Jan 28, 2022

This was referenced Jan 28, 2022

Use Sock in CNode #23604

Merged

lib: Optimizing siphash implementation #18014

Closed

PastaPastaPasta mentioned this pull request Jan 31, 2022

refactor: use Span in random.* #24213

Merged

PastaPastaPasta mentioned this pull request Feb 1, 2022

docs: avoid C-style casts; use modern C++ casts #23810

Merged

DrahtBot added the Needs rebase label Feb 4, 2022

PastaPastaPasta force-pushed the explicit-reinterpret-cast branch from 8bff802 to 73da5aa Compare April 7, 2022 05:01

DrahtBot removed the Needs rebase label Apr 7, 2022

DrahtBot added the Needs rebase label Apr 19, 2022

PastaPastaPasta force-pushed the explicit-reinterpret-cast branch from 73da5aa to 46a9a73 Compare April 19, 2022 18:44

DrahtBot removed the Needs rebase label Apr 19, 2022

This was referenced Apr 20, 2022

refactor: make GetRand a template, remove GetRandInt #24925

Merged

Isolate the storage abstraction layer from the application/serialization layer #24922

Closed

DrahtBot added the Needs rebase label Apr 21, 2022

PastaPastaPasta force-pushed the explicit-reinterpret-cast branch from 46a9a73 to cb2d322 Compare April 21, 2022 15:14

DrahtBot removed the Needs rebase label Apr 21, 2022

This was referenced Apr 25, 2022

prevector: enforce is_trivially_copyable_v #24962

Merged

tidy: modernize-use-nullptr #24971

Merged

DrahtBot added the Needs rebase label Apr 26, 2022

PastaPastaPasta added 2 commits May 11, 2022 13:12

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

bb0f8d7

… casts

refactor: replace all implicit C-style const/const+reinterpret with e…

96cc421

…xplicit casts

PastaPastaPasta force-pushed the explicit-reinterpret-cast branch from cb2d322 to 96cc421 Compare May 11, 2022 18:16

DrahtBot removed the Needs rebase label May 11, 2022

DrahtBot added the Needs rebase label May 12, 2022

DrahtBot mentioned this pull request May 12, 2022

tracing: lock contention analysis #25081

Closed

achow101 closed this Oct 12, 2022

bitcoin locked and limited conversation to collaborators Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PastaPastaPasta commented Jan 28, 2022

DrahtBot commented Jan 28, 2022 •

edited

Loading

PastaPastaPasta commented Jan 31, 2022

maflcko commented Jan 31, 2022

PastaPastaPasta commented Jan 31, 2022

maflcko commented Jan 31, 2022

jonatack commented Jan 31, 2022

martinus commented Apr 27, 2022

maflcko commented Apr 27, 2022

martinus commented Apr 27, 2022

DrahtBot commented May 12, 2022

achow101 commented Oct 12, 2022

PastaPastaPasta commented Oct 12, 2022

martinus commented Oct 13, 2022

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

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

Conversation

PastaPastaPasta commented Jan 28, 2022

DrahtBot commented Jan 28, 2022 • edited Loading

Conflicts

PastaPastaPasta commented Jan 31, 2022

maflcko commented Jan 31, 2022

PastaPastaPasta commented Jan 31, 2022

maflcko commented Jan 31, 2022

jonatack commented Jan 31, 2022

martinus commented Apr 27, 2022

maflcko commented Apr 27, 2022

martinus commented Apr 27, 2022

DrahtBot commented May 12, 2022

achow101 commented Oct 12, 2022

PastaPastaPasta commented Oct 12, 2022

martinus commented Oct 13, 2022

DrahtBot commented Jan 28, 2022 •

edited

Loading