search

sipa / minisketch

Minisketch: an optimized library for BCH-based set reconciliation
MIT License
310 stars 52 forks source link

refactor: remove all c style casts in minisketch subtree #57

Open PastaPastaPasta opened 2 years ago

PastaPastaPasta commented 2 years ago

A part of https://github.com/bitcoin/bitcoin/pull/23810

sipa commented 2 years ago

I don't really see the point of using C++ style casts for integer types, as C-style casts are equivalent for those anyway.

PastaPastaPasta commented 2 years ago

I don't really see the point of using C++ style casts for integer types, as C-style casts are equivalent for those anyway.

Yeah, if we could use C style casts for only integer types I'd be totally happy. The problem comes when using C style casts with more complex (and potentially dangerous) types.

The main reason for this PR, is that it allows us to use tooling (see -Wold-style-cast) to prevent, in Bitcoin Core or other libraries that use minisketch, the dangerous usage of C style casts. If you look at the bitcoin PR, I replaced a LOT of C style casts that were acting as a reinterpret_cast as opposed to a normal static_cast.

I speak more about the benefits of this change / enforcement of no C style casts in the bitcoin PR

maflcko commented 2 years ago

I know nothing about the build system, but I think it is possible to disable warnings for subtrees?

PastaPastaPasta commented 2 years ago

Please see latest @sipa

I have replaced non-narrowing static_casts (I introduced) with c++11 braced init instead

PastaPastaPasta commented 2 years ago

@sipa can you please review the current state of this PR?

Thanks

PastaPastaPasta commented 2 years ago

All done, will squash if requested

PastaPastaPasta commented 2 years ago

@sipa how does that look?

PastaPastaPasta commented 1 year ago

Hi going through my old open PRs. Any chance we can get this merged?