Skip to content
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

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

PastaPastaPasta
Copy link
Contributor

@PastaPastaPasta PastaPastaPasta commented Dec 18, 2021

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.

@fanquake
Copy link
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.

@PastaPastaPasta
Copy link
Contributor Author

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
Copy link
Member

sipa commented Dec 18, 2021

-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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

-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

@sipa
Copy link
Member

sipa commented Feb 1, 2022

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

@PastaPastaPasta
Copy link
Contributor Author

@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
Copy link
Member

sipa commented Feb 1, 2022

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 PastaPastaPasta changed the title refactor: destroy all C-style casts; use modern C++ casts, enforce via -Wold-style-cast docs: avoid C-style casts; use modern C++ casts Feb 1, 2022
@PastaPastaPasta
Copy link
Contributor Author

PastaPastaPasta commented Feb 1, 2022

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

doc/developer-notes.md Outdated Show resolved Hide resolved
Copy link

@uvhw uvhw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@maflcko
Copy link
Member

maflcko commented Aug 1, 2022

Can this be closed?

@PastaPastaPasta
Copy link
Contributor Author

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

@maflcko
Copy link
Member

maflcko commented Aug 2, 2022

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.

Copy link
Member

@maflcko maflcko left a 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++"

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jan 5, 2023
…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
@fanquake
Copy link
Member

fanquake commented Feb 8, 2023

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 fanquake merged commit 8d69b61 into bitcoin:master Feb 8, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 8, 2023
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
@PastaPastaPasta PastaPastaPasta deleted the c-style-cast branch February 19, 2023 17:44
@bitcoin bitcoin locked and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.