-
Notifications
You must be signed in to change notification settings - Fork 36.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: avoid C-style casts; use modern C++ casts #23810
Conversation
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. |
This PR wont be merged with the reversion commits; you need to remove all the subtree changes from the first commit. |
2628c28
to
5491807
Compare
Yeah, that's fair. Simply dropped the sub-tree commits |
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:
Cons:
completely disallow C-style casts, and use c++ casts.Pros:
Cons:
|
5491807
to
cfdb1e7
Compare
8ee3df7
to
d351616
Compare
@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. |
d351616
to
bdb888b
Compare
-Wold-style-cast
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.
Note: you can see the old branch here https://github.com/PastaPastaPasta/dash/tree/c-style-cast2 |
bdb888b
to
7534723
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, with the understanding that in practice this will be handled just like the previous rule "++i
is preferred over i++
"
…terals instead of c style casts f2fc03e refactor: use braced init for integer constants instead of c style casts (Pasta) Pull request description: See bitcoin/bitcoin#23810 for more context. This is broken out from that PR, as it is less breaking, and should be trivial to review and merge. EDIT: Long term, the intention is to remove all C-style casts, as they can dangerously introduce reinterpret_casts. This is one step which removes a number of trivially removable C-style casts ACKs for top commit: aureleoules: ACK f2fc03e Tree-SHA512: 2fd11b92c9147e3f970ec3e130e3b3dce70e707ff02950a8c697d4b111ddcbbfa16915393db20cfc8f384bc76f13241c9b994a187987fcecd16a61f8cc0af14c
Going to merge this, as we've started moving this direction (#23829). However agree with:
|
7534723 docs: document c-style cast prohibition (Pasta) Pull request description: 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. Top commit has no ACKs. Tree-SHA512: 29a98de396f0c78e32d8a1831319162203c4405a670da5add5da956fcc7df200a1cec162ef1cfac4ddfb02714b66406081d40ed435c7f0f28581cfa24d94fac1
In the words of practicalswift:
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.