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

In the words of practicalswift:

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

By using static_cast<T>(...) explicitly we avoid the possibility of an unintentional and 
dangerous reinterpret_cast. Furthermore static_cast<T>(...) allows for easier grepping of casts.

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

Modern tooling, specifically -Wold-style-cast can enable us to enforce never using C-style casts. I believe this is especially important due to the number of C-style casts the codebase is currently being used as a reinterpret_cast. reinterpret_casts are especially dangerous, and should never be done via C-style casts.

Update the docs to suggest the use of named cast or functional casts.

Yes any change to a subtree needs to be PR'd to the relevant subtree. The subtrees are listed here: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees.

Yes any change to a subtree needs to be PR'd to the relevant subtree. The subtrees are listed here: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees.

Thanks, have opened PRs to relevant sub-trees. I've also reverted the subtree changes present in this PR, such that CI will hopefully be happy. (although it also required reverting the -Wold-style-cast commit)

-0

I'm generally not a fan of using C++ style casts for integer types. For those, it's just overly verbose for a simple operation with no gain. I understand the guideline doesn't make this distinction, but I very much wish it did.

I can see there being some advantage to being able to use the compiler flag to guarantee no C-style casts remain in places where it actually might be dangerous, but it's unfortunate there is no way to restrict it to more complex types. I could be convinced this is worth changing it everywhere just to get that benefit, but it's a hard sell. This is also a big change that touches lots of pieces of code.

I've also reverted the subtree changes present in this PR,

This PR wont be merged with the reversion commits; you need to remove all the subtree changes from the first commit.

I've also reverted the subtree changes present in this PR,

This PR wont be merged with the reversion commits; you need to remove all the subtree changes from the first commit.

Yeah, that's fair. Simply dropped the sub-tree commits

-0

I'm generally not a fan of using C++ style casts for integer types. For those, it's just overly verbose for a simple operation with no gain. I understand the guideline doesn't make this distinction, but I very much wish it did.

I can see there being some advantage to being able to use the compiler flag to guarantee no C-style casts remain in places where it actually might be dangerous, but it's unfortunate there is no way to restrict it to more complex types. I could be convinced this is worth changing it everywhere just to get that benefit, but it's a hard sell. This is also a big change that touches lots of pieces of code.

As I see it, we have two options, either

allow C-style casts and try to maintain a rule that says "if it's a integer type, then you can use C style casts" otherwise use c++ casts.

Pros:

• C style casts are second nature for experienced c++ devs
• take less key strokes

Cons:

• Must be enforced by reviewers
• Can introduce silent reinterpret_casts and other potentially dangerous casts
• Almost impossible to effectively grep for
• Can appear ambiquous / less readable
  • (int) x + 3.0 isn't always the clearest especially in more complex code (does cast apply to (int)(x+3.0) or (int)(x) + 3.0)

completely disallow C-style casts, and use c++ casts.

Pros:

• Verbose
• easily grep-able for any type of cast
• Allows for tooling to enforce out rules (-Wold-style-cast) instead of reviewers
• Perfectly clear what is being casted and how
• Doesn't allow unintended dangerous casts

Cons:

• More characters
• Not second nature

Sorry for using CI for debugging.. I'm trying to cross-build from focal, and not getting these errors... I presume I'm not actually building for windows?

./configure HOST=x86_64-w64-mingw32 --enable-werror
make clean && make -j16 HOST=x86_64-w64-mingw32

./configure HOST=x86_64-w64-mingw32 --enable-werror

You want --host=x86_64-w64-mingw32 not HOST=.

-0 in general for the reasons sipa gives. Personally, I try to use C++11 braces {} for integer conversion where possible. If that isn't possible, I use normal (). So it should already be pretty clear where the cast is narrowing and where it isn't.

So +0 on using {}, but -0 on using the verbose casts for ints. +0 for using verbose casts for pointers, since we usually get those wrong (https://github.com/bitcoin/bitcoin/pull/23438#discussion_r742947533, https://github.com/bitcoin/bitcoin/pull/23810/files#r771801088).

Must be enforced by reviewers

Not implying anything, but in theory it would be possible to write tooling using the clang-tidy backend.

I'm generally not a fan of using C++ style casts for integer types. For those, it's just overly verbose for a simple operation with no gain. I understand the guideline doesn't make this distinction, but I very much wish it did.

Same tbh. I also dislike all-over-the-place commits. This is virtually impossible to review and will yet again need everyone with an existing patch, who does more significant work, to rebase.

So tend toward NACK.

TBH, I'm doubt if a thorough review of changes in 137 files is doable.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

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.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

would be possible to write tooling using the clang-tidy backend.

I'd be quite happy with this as a solution, sadly I don't see that really happening any time soon. From my investigation, it appeared non-trivial.

Personally, I try to use C++11 braces {} for integer conversion where possible. If that isn't possible, I use normal (). So it should already be pretty clear where the cast is narrowing and where it isn't.

I would be totally happy with this (and could probably start work on this) and, it also satisfies the compiler -Wold-style-cast. It would just be an amount of work (although not as bad since static_casts are now greppable :) )

Personally, I try to use C++11 braces {} for integer conversion where possible. If that isn't possible, I use normal (). So it should already be pretty clear where the cast is narrowing and where it isn't.

TBH, I'm doubt if a thorough review of changes in 137 files is doable.

Although sure, this does touch a lot of files, it doesn't touch many lines. Only ~700 lines, which is not a huge PR as far as this repo is concerned. I could also break this out into multiple PRs, ex. (one for src/test/; one for src/crypto; one for src/wallet; etc) such that it is easier to review, and less breaking.

EDIT: I have broken up the commit into a number of commits, one for reinterpret, one for const/const+reint, one for bracket init, and then a few for remaining static casts in various parts of the codebase. I think at least merging the reinterpret and const/const+reint commit is valuable.

I've updated https://github.com/bitcoin/bitcoin/pull/23810/commits/8ef3ac9270e94e70730ed7ed5b106c2a89278f8b and https://github.com/bitcoin/bitcoin/pull/23810/commits/560628adc305b6c170d6d2f0ce46ad2fbd676c93 to avoid static_casts.

If both the cast type and castee are integers, then I'm using brace / paren init. Otherwise static_cast

Maybe split the one commit 8129eba176a6d16c95461440393431424201e056 up into a separate pull, assuming that it doesn't change the binary and doesn't cause conflicts, it might be acceptable?

-0 on the rest for the reasons already given. Splitting up commits doesn't change the reason why they should be made/not made.

Maybe split the one commit 8129eba up into a separate pull, assuming that it doesn't change the binary and doesn't cause conflicts, it might be acceptable?

Done @MarcoFalke

I think that it makes sense to merge these sorts of 'big automated refactors' that have a list of conflicts wronger than a CVS receipt right before we cut a release, and after feature freeze, since they can be done and reviewed easily with automated tooling, that workflow minimizes aggrevation and rebasing needed to be done to ship features during a release cycle.

based on: #22969

this would mean that after/on 2022-02-15 is a reasonable time to merge.

Rebased to resolve conflicts

note, over 23 days of merged PRs, there were only a few conflicts. Most of which CLion was able to automatically deduce the correct merge for me. Two other conflicts weren't automatic, but were trivial to resolve. I don't think merging this PR would cause substantial effort on other contributors.

NACK along the lines of: if it ain't ugly, slow or broke, don't fix it.

NACK along the lines of: if it ain't ugly, slow or broke, don't fix it.

It is very ugly. Please see all the information I've posted in this PR about why this is important and let me know what you disagree with.

Specifcally "This PR converts 38 subtle const_cast's and 187 subtle reinterpret_cast's into explicit grep-able casts!"

Implicit const and reinterpret_casts are very ugly and should be removed.

@PastaPastaPasta Please don't spam the repository nagging people to follow the policy you're trying to institute here.

@PastaPastaPasta Please don't spam the repository nagging people to follow the policy you're trying to institute here. @sipa It seemed to me that the consensus was that moving away from c-style casts was generally good, but needs to be done incrementally as other PRs change the code. This is as was suggested by jonatack #24185 (comment), and seems to be what MarcoFalke wants moving forward (as many of his PRs have changed c-style casts into functional casts).

I don't see what the problem is with me reviewing PRs, and adding relevant suggestions that the author can either apply or not. I've given suggestions unrelated to "don't use c-style casts", as I've been looking for those c-style casts, I've been reviewing the PR in general. I understand that it may seem "spammy", but there are a lot of PRs that affect code containing c-style casts, and if possible, and if the PR author agrees, I'd like for those PRs to also change those c-style casts to better casts.

It seems to me that there is a degree of agreement that moving away from c-style casts is valuable. As such, there are two options.

If we want to do this change over multiple years and iteratively, that's fine, I'll ask PR authors as I review their PR to change c-style casts. If we want to do this in larger blocks, then I can create PRs that are as non-breaking as possible so that we can merge them, but it seems that there is consensus that we don't want to do this in larger blocks.

Based on the comments here, it is far from clear to me there is agreement we should have this policy at all. I personally don't see a problem for example with C-style casts for integer data types - they're far more readable than the equivalent C++ style casts for those, and their meaning is clear enough. I don't object to instituting a policy, and following it, if we agree to follow it as a project, and to what extent to enforce it, but I'm not convinced that will happen, and if it doesn't, I'd consider nagging unrelated pull requests about it as spam, distracting authors and reviewers from more important matters.

I have updated this PR to simply be focused on adding a comment to developer-notes.md. I think this guideline is pretty reasonable. Please provide comments.

  - Use a named cast or functional cast, not a C-Style cast. When casting
    between integer types, use functional casts such as `int(x)` or `int{x}`
    instead of `(int) x`. When casting between more complex types, use static_cast.
    Use reinterpret_cast and const_cast as appropriate.

Note: you can see the old branch here https://github.com/PastaPastaPasta/dash/tree/c-style-cast2

:+1:

<!-- 1GwvLW9qJ8uaYjew3cFvPiqxViWhuU1pKT:BTC150 -> <!-:1st_place_medal: ->

Can this be closed?

It can be merged? I think merging this into docs and establishing as general rule is good and has no downsides

Oh I assumed this pull had no ACKs, and only NACKs, in which case merging is not possible. Though, it seems the latest push changed this from cpp changes to a doc change.

ACK, with the understanding that in practice this will be handled just like the previous rule "++i is preferred over i++"

Going to merge this, as we've started moving this direction (#23829). However agree with:

with the understanding that in practice this will be handled just like the previous rule "++i is preferred over i++"