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

PastaPastaPasta commented at 4:41 am on December 18, 2021: contributor

In the words of practicalswift:

 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
 8By using static_cast<T>(...) explicitly we avoid the possibility of an unintentional and 
 9dangerous reinterpret_cast. Furthermore static_cast<T>(...) allows for easier grepping of casts.
10
11For a more thorough discussion, see "ES.49: If you must use a cast, use a named cast"
12in 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.

fanquake commented at 4:44 am on December 18, 2021: member

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.

DrahtBot added the label Block storage on Dec 18, 2021

DrahtBot added the label Build system on Dec 18, 2021

DrahtBot added the label Consensus on Dec 18, 2021

DrahtBot added the label Descriptors on Dec 18, 2021

DrahtBot added the label Mempool on Dec 18, 2021

DrahtBot added the label Mining on Dec 18, 2021

DrahtBot added the label P2P on Dec 18, 2021

DrahtBot added the label Refactoring on Dec 18, 2021

DrahtBot added the label RPC/REST/ZMQ on Dec 18, 2021

DrahtBot added the label TX fees and policy on Dec 18, 2021

DrahtBot added the label Upstream on Dec 18, 2021

DrahtBot added the label Utils/log/libs on Dec 18, 2021

DrahtBot added the label UTXO Db and Indexes on Dec 18, 2021

DrahtBot added the label Validation on Dec 18, 2021

DrahtBot added the label Wallet on Dec 18, 2021

PastaPastaPasta commented at 5:33 am on December 18, 2021: contributor

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)

sipa commented at 5:39 am on December 18, 2021: member

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

fanquake commented at 5:42 am on December 18, 2021: member

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.

PastaPastaPasta force-pushed on Dec 18, 2021

PastaPastaPasta commented at 5:52 am on December 18, 2021: contributor

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

PastaPastaPasta commented at 6:09 am on December 18, 2021: contributor

-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

PastaPastaPasta force-pushed on Dec 18, 2021

fanquake removed the label Wallet on Dec 18, 2021

fanquake removed the label Build system on Dec 18, 2021

fanquake removed the label TX fees and policy on Dec 18, 2021

fanquake removed the label UTXO Db and Indexes on Dec 18, 2021

fanquake removed the label RPC/REST/ZMQ on Dec 18, 2021

fanquake removed the label P2P on Dec 18, 2021

fanquake removed the label Mining on Dec 18, 2021

fanquake removed the label Validation on Dec 18, 2021

fanquake removed the label Mempool on Dec 18, 2021

fanquake removed the label Consensus on Dec 18, 2021

fanquake removed the label Block storage on Dec 18, 2021

fanquake removed the label Utils/log/libs on Dec 18, 2021

fanquake removed the label Descriptors on Dec 18, 2021

PastaPastaPasta force-pushed on Dec 18, 2021

PastaPastaPasta commented at 7:10 am on December 18, 2021: contributor

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?

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

fanquake commented at 7:14 am on December 18, 2021: member

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

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

in src/script/interpreter.cpp:1306 in 74fc3e62fd outdated

1302@@ -1303,12 +1303,12 @@ class CTransactionSignatureSerializer
1303         it = itBegin;
1304         while (scriptCode.GetOp(it, opcode)) {
1305             if (opcode == OP_CODESEPARATOR) {
1306-                s.write((char*)&itBegin[0], it-itBegin-1);
1307+                s.write(reinterpret_cast<const char*>(&itBegin[0]), it-itBegin-1);

maflcko commented at 8:42 am on December 18, 2021:

This is already fixed in https://github.com/bitcoin/bitcoin/pull/23438/files#diff-a0337ffd7259e8c7c9a7786d6dbd420c80abfa1afdb34ebae3261109d9ae3c19

maflcko commented at 8:43 am on December 18, 2021: member

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

maflcko commented at 8:46 am on December 18, 2021: member

Must be enforced by reviewers

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

laanwj commented at 9:34 am on December 18, 2021: member

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.

hebasto commented at 12:35 pm on December 18, 2021: member

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

DrahtBot commented at 4:37 pm on December 18, 2021: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	laanwj, jb55
Concept ACK	MarcoFalke

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.

PastaPastaPasta commented at 5:35 pm on December 18, 2021: contributor

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.

PastaPastaPasta force-pushed on Dec 18, 2021

PastaPastaPasta commented at 9:19 pm on December 18, 2021: contributor

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

maflcko commented at 8:54 am on December 20, 2021: member

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.

PastaPastaPasta commented at 6:02 pm on December 20, 2021: contributor

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

PastaPastaPasta force-pushed on Dec 20, 2021

JeremyRubin commented at 9:05 pm on December 20, 2021: contributor

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.

DrahtBot added the label Needs rebase on Jan 4, 2022

PastaPastaPasta force-pushed on Jan 10, 2022

PastaPastaPasta commented at 2:33 am on January 10, 2022: contributor

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.

DrahtBot removed the label Needs rebase on Jan 10, 2022

DrahtBot added the label Needs rebase on Jan 10, 2022

PastaPastaPasta force-pushed on Jan 10, 2022

DrahtBot removed the label Needs rebase on Jan 10, 2022

DrahtBot added the label Needs rebase on Jan 13, 2022

jb55 commented at 8:00 pm on January 13, 2022: contributor

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

PastaPastaPasta force-pushed on Jan 14, 2022

PastaPastaPasta commented at 0:27 am on January 14, 2022: contributor

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.

DrahtBot removed the label Needs rebase on Jan 14, 2022

DrahtBot added the label Needs rebase on Jan 27, 2022

PastaPastaPasta force-pushed on Jan 28, 2022

DrahtBot removed the label Needs rebase on Jan 28, 2022

sipa commented at 3:19 pm on February 1, 2022: member

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

PastaPastaPasta commented at 4:12 pm on February 1, 2022: contributor

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

sipa commented at 4:18 pm on February 1, 2022: member

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.

PastaPastaPasta force-pushed on Feb 1, 2022

PastaPastaPasta renamed this:
~~refactor: destroy all C-style casts; use modern C++ casts, enforce via `-Wold-style-cast`~~
docs: avoid C-style casts; use modern C++ casts
on Feb 1, 2022

PastaPastaPasta commented at 4:39 pm on February 1, 2022: contributor

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.

0  - Use a named cast or functional cast, not a C-Style cast. When casting
1    between integer types, use functional casts such as `int(x)` or `int{x}`
2    instead of `(int) x`. When casting between more complex types, use static_cast.
3    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

in doc/developer-notes.md:87 in bdb888b2e1 outdated

80@@ -81,6 +81,10 @@ tool to clean up patches automatically before submission.
81     function declarations over multiple lines using the Clang Format
82     [AlignAfterOpenBracket](https://clang.llvm.org/docs/ClangFormatStyleOptions.html)
83     style option.
84+  - Use a named cast or functional cast, not a C-Style cast. When casting
85+    between integer types, use functional casts such as `int(x)` or `int{x}`
86+    instead of `(int) x`. When casting between more complex types, use static_cast.
87+    Use reinterpret_cast and const_cast as appropriate.

prusnak commented at 5:09 pm on February 1, 2022:

You got the alignment wrong - there is one extra space at start of each line.

PastaPastaPasta commented at 5:18 pm on February 1, 2022:

I don’t think so… Should be preceded by four spaces right?

PastaPastaPasta commented at 5:32 pm on February 1, 2022:

I see what you mean now. I think I’ve moved it into the correct place now

prusnak changes_requested

docs: document c-style cast prohibition 75347236f2

PastaPastaPasta force-pushed on Feb 1, 2022

uvhw approved

uvhw commented at 9:55 am on April 5, 2022: none

:+1:

maflcko commented at 9:36 am on August 1, 2022: member

Can this be closed?

PastaPastaPasta commented at 4:19 pm on August 1, 2022: contributor

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

maflcko commented at 7:28 am on August 2, 2022: member

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.

maflcko approved

maflcko commented at 7:30 am on August 2, 2022: member

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

maflcko referenced this in commit 3212d104f4 on Jan 5, 2023

fanquake removed the label Refactoring on Feb 8, 2023

fanquake removed the label Upstream on Feb 8, 2023

fanquake added the label Docs on Feb 8, 2023

fanquake commented at 10:36 am on February 8, 2023: member

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++”

fanquake merged this on Feb 8, 2023

fanquake closed this on Feb 8, 2023

sidhujag referenced this in commit 52c53ea57d on Feb 8, 2023

PastaPastaPasta deleted the branch on Feb 19, 2023

bitcoin locked this on Feb 19, 2024

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

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.

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

Reviews

Conflicts