"implicit conversion loses integer precision" warnings

hebasto commented at 11:21 am on October 15, 2024: member

When building with clang and the -Wshorten-64-to-32 flag, there are some warnings:

 0In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:27:
 1In file included from /home/hebasto/git/secp256k1/secp256k1/src/field_impl.h:14:
 2In file included from /home/hebasto/git/secp256k1/secp256k1/src/field_5x52_impl.h:13:
 3/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:279:39: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
 4  279 |             w = (f * g * (f * f - 2)) & m;
 5      |               ~ ~~~~~~~~~~~~~~~~~~~~~~^~~
 6/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:289:19: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
 7  289 |             w = f + (((f + 1) & 4) << 1);
 8      |               ~ ~~^~~~~~~~~~~~~~~~~~~~~~
 9/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:290:26: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
10  290 |             w = (-w * g) & m;
11      |               ~ ~~~~~~~~~^~~
12/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:370:39: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
13  370 |             w = (f * g * (f * f - 2)) & m;
14      |               ~ ~~~~~~~~~~~~~~~~~~~~~~^~~
15/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:380:19: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
16  380 |             w = f + (((f + 1) & 4) << 1);
17      |               ~ ~~^~~~~~~~~~~~~~~~~~~~~~
18/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:381:26: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
19  381 |             w = (-w * g) & m;
20      |               ~ ~~~~~~~~~^~~
21In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:28:
22In file included from /home/hebasto/git/secp256k1/secp256k1/src/scalar_impl.h:20:
23/home/hebasto/git/secp256k1/secp256k1/src/scalar_4x64_impl.h:119:42: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
24  119 |     overflow = secp256k1_u128_to_u64(&t) + secp256k1_scalar_check_overflow(r);
25      |              ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
26/home/hebasto/git/secp256k1/secp256k1/src/scalar_4x64_impl.h:682:34: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'unsigned int' [-Wshorten-64-to-32]
27  682 |     secp256k1_scalar_reduce(r, c + secp256k1_scalar_check_overflow(r));
28      |     ~~~~~~~~~~~~~~~~~~~~~~~    ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
29In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:30:
30/home/hebasto/git/secp256k1/secp256k1/src/ecmult_impl.h:518:21: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
31  518 |     for (i = n_wnaf - 1; i >= 0; i--) {
32      |            ~ ~~~~~~~^~~
33/home/hebasto/git/secp256k1/secp256k1/src/ecmult_impl.h:562:52: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
34  562 |         for(j = ECMULT_TABLE_SIZE(bucket_window+2) - 1; j > 0; j--) {
35      |               ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
36In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:32:
37In file included from /home/hebasto/git/secp256k1/secp256k1/src/ecmult_gen_impl.h:14:
38/home/hebasto/git/secp256k1/secp256k1/src/hash_impl.h:151:52: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
39  151 |     secp256k1_write_be32(&sizedesc[0], hash->bytes >> 29);
40      |     ~~~~~~~~~~~~~~~~~~~~               ~~~~~~~~~~~~^~~~~
41/home/hebasto/git/secp256k1/secp256k1/src/hash_impl.h:152:52: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
42  152 |     secp256k1_write_be32(&sizedesc[4], hash->bytes << 3);
43      |     ~~~~~~~~~~~~~~~~~~~~               ~~~~~~~~~~~~^~~~
44/home/hebasto/git/secp256k1/secp256k1/src/hash_impl.h:261:19: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
45  261 |         int now = outlen;
46      |             ~~~   ^~~~~~
4713 warnings generated.

real-or-random commented at 11:35 am on October 15, 2024: contributor

When building with clang and the -Wshorten-64-to-32 flag, there are some warnings:

Well, then don’t use this flag. :P Or do you think this is a problem? Is it a default flag?

hebasto commented at 12:11 pm on October 15, 2024: member

Or do you think this is a problem?

Each case has to be investigated to answer this question.

Is it a default flag?

macOS Xcode set it by default.

real-or-random added the label refactor/smell on Oct 15, 2024

real-or-random commented at 3:22 pm on November 7, 2024: contributor

I’m a bit on the fence. All of these are fixable, and maybe some should be fixed (in particular the last one), but in other cases, this will just use up some of the precious reviewer bandwidth.

 0In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:27:
 1In file included from /home/hebasto/git/secp256k1/secp256k1/src/field_impl.h:14:
 2In file included from /home/hebasto/git/secp256k1/secp256k1/src/field_5x52_impl.h:13:
 3/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:279:39: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
 4  279 |             w = (f * g * (f * f - 2)) & m;
 5      |               ~ ~~~~~~~~~~~~~~~~~~~~~~^~~
 6/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:289:19: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
 7  289 |             w = f + (((f + 1) & 4) << 1);
 8      |               ~ ~~^~~~~~~~~~~~~~~~~~~~~~
 9/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:290:26: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
10  290 |             w = (-w * g) & m;
11      |               ~ ~~~~~~~~~^~~
12/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:370:39: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
13  370 |             w = (f * g * (f * f - 2)) & m;
14      |               ~ ~~~~~~~~~~~~~~~~~~~~~~^~~
15/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:380:19: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
16  380 |             w = f + (((f + 1) & 4) << 1);
17      |               ~ ~~^~~~~~~~~~~~~~~~~~~~~~
18/home/hebasto/git/secp256k1/secp256k1/src/modinv64_impl.h:381:26: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
19  381 |             w = (-w * g) & m;
20      |               ~ ~~~~~~~~~^~~

These are harmless, but it may be more natural to make w a uint64_t? @sipa

0In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:28:
1In file included from /home/hebasto/git/secp256k1/secp256k1/src/scalar_impl.h:20:
2/home/hebasto/git/secp256k1/secp256k1/src/scalar_4x64_impl.h:119:42: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
3  119 |     overflow = secp256k1_u128_to_u64(&t) + secp256k1_scalar_check_overflow(r);
4      |              ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5/home/hebasto/git/secp256k1/secp256k1/src/scalar_4x64_impl.h:682:34: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'unsigned int' [-Wshorten-64-to-32]
6  682 |     secp256k1_scalar_reduce(r, c + secp256k1_scalar_check_overflow(r));
7      |     ~~~~~~~~~~~~~~~~~~~~~~~    ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think these warnings are a bit stupid, but okay, explicit is better than implicit. I wouldn’t mind adding an explicit cast.

0In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:30:
1/home/hebasto/git/secp256k1/secp256k1/src/ecmult_impl.h:518:21: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
2  518 |     for (i = n_wnaf - 1; i >= 0; i--) {
3      |            ~ ~~~~~~~^~~
4/home/hebasto/git/secp256k1/secp256k1/src/ecmult_impl.h:562:52: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
5  562 |         for(j = ECMULT_TABLE_SIZE(bucket_window+2) - 1; j > 0; j--) {
6      |               ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
7```

I don’t know. We could make all of these size_t or ptrdiff_t if we care about negative values. Does this improve the code? Perhaps, but it’s not entirely trivial because it needs work and review.

0In file included from /home/hebasto/git/secp256k1/secp256k1/src/secp256k1.c:32:
1In file included from /home/hebasto/git/secp256k1/secp256k1/src/ecmult_gen_impl.h:14:
2/home/hebasto/git/secp256k1/secp256k1/src/hash_impl.h:151:52: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
3  151 |     secp256k1_write_be32(&sizedesc[0], hash->bytes >> 29);
4      |     ~~~~~~~~~~~~~~~~~~~~               ~~~~~~~~~~~~^~~~~
5/home/hebasto/git/secp256k1/secp256k1/src/hash_impl.h:152:52: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
6  152 |     secp256k1_write_be32(&sizedesc[4], hash->bytes << 3);
7      |     ~~~~~~~~~~~~~~~~~~~~               ~~~~~~~~~~~~^~~~

These are obviously okay. Happy to add a cast if that makes apple clang happy…

0/home/hebasto/git/secp256k1/secp256k1/src/hash_impl.h:261:19: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
1  261 |         int now = outlen;
2      |             ~~~   ^~~~~~

Okay, now should be a size_t here. This is not a problem in the current code base because the function is always called with outlen == 32.

“implicit conversion loses integer precision” warnings #1617