refactor: remove all c style casts in minisketch subtree #57
Conversation
|
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 I speak more about the benefits of this change / enforcement of no C style casts in the bitcoin PR |
|
I know nothing about the build system, but I think it is possible to disable warnings for subtrees? |
|
Please see latest @sipa I have replaced non-narrowing static_casts (I introduced) with c++11 braced init instead |
src/test.cpp
Outdated
| @@ -289,7 +289,7 @@ int main(int argc, char** argv) { | |||
| #else | |||
| const char* mode = ""; | |||
| #endif | |||
| printf("Running libminisketch tests%s with complexity=%llu\n", mode, (unsigned long long)test_complexity); | |||
| printf("Running libminisketch tests%s with complexity=%lu\n", mode, uint64_t{test_complexity}); | |||
There was a problem hiding this comment.
I think if we want this to be unsigned long long and not uint64, then I'll need to use a static_cast (since it's a multiple word identifier). Up to you
There was a problem hiding this comment.
This isn't correct. uint64_t isn't the same as unsigned long on all platforms.
PastaPastaPasta
force-pushed
the
c-style-cast
branch
from
8d5492d
to
3ea0368
Compare
PastaPastaPasta
force-pushed
the
c-style-cast
branch
from
3ea0368
to
64367c8
Compare
|
@sipa can you please review the current state of this PR? Thanks |
src/fields/clmul_common_impl.h
Outdated
| @@ -33,7 +33,7 @@ template<typename I, int BITS, I MOD> NO_SANITIZE_MEMORY I MulWithClMulReduce(I | |||
| static constexpr I MASK = Mask<BITS, I>(); | |||
|
|
|||
| const __m128i MOD128 = _mm_cvtsi64_si128(MOD); | |||
| __m128i product = _mm_clmulepi64_si128(_mm_cvtsi64_si128((uint64_t)a), _mm_cvtsi64_si128((uint64_t)b), 0x00); | |||
| __m128i product = _mm_clmulepi64_si128(_mm_cvtsi64_si128(uint64_t(a)), _mm_cvtsi64_si128(uint64_t(b)), 0x00); | |||
There was a problem hiding this comment.
I think all of these can be uint64_t{a} etc.
src/fields/clmul_common_impl.h
Outdated
| @@ -143,7 +143,7 @@ template<typename I, int B, I MOD, I (*MUL)(I, I), typename F, const F* SQR, con | |||
| Elem FromSeed(uint64_t seed) const { | |||
| uint64_t k0 = 0x434c4d554c466c64ull; // "CLMULFld" | |||
| uint64_t k1 = seed; | |||
| uint64_t count = ((uint64_t)B) << 32; | |||
| uint64_t count = (uint64_t(B)) << 32; | |||
There was a problem hiding this comment.
Can be uint64_t{B}, and then doesn't need parenthesis around it anymore.
src/test.cpp
Outdated
| @@ -289,7 +289,7 @@ int main(int argc, char** argv) { | |||
| #else | |||
| const char* mode = ""; | |||
| #endif | |||
| printf("Running libminisketch tests%s with complexity=%llu\n", mode, (unsigned long long)test_complexity); | |||
| printf("Running libminisketch tests%s with complexity=%llu\n", mode, static_cast<unsigned long long>(test_complexity)); | |||
There was a problem hiding this comment.
You can use (unsigned long long){test_complexity} here to avoid a C-style case (and prevent narrowing at the same time!).
|
All done, will squash if requested |
|
@sipa how does that look? |
|
Hi going through my old open PRs. Any chance we can get this merged? |
A part of bitcoin/bitcoin#23810