Use size_t shifts when computing a size_t #607

pull sipa wants to merge 1 commits into bitcoin-core:master from sipa:201903_sizetshift changing 1 files +3 −3

sipa commented at 7:57 PM on March 30, 2019: contributor

This was detected by compiling with MSVC; it triggers warning C4334.

I don't think this is necessary, as we know the maximum shift is a very small integer, but this makes the code more obviously correct.
practicalswift commented at 9:00 PM on March 30, 2019: contributor
utACK 1b8e50570c7227d958989d16fafa40313073a462

Rationale:
1. The change makes the code more obviously correct as noted.
2. The change increases the signal to noise ratio in compiler/static analyser output which makes it easier to spot real issues.
sipa force-pushed on Mar 30, 2019
sipa commented at 9:17 PM on March 30, 2019: contributor
Updated. Suggestion from IRC:
```
< gmaxwell> sipa: why multiply instead shifting the sizeof?
```

in src/ecmult_impl.h:1066 in a064a2ba29 outdated

1062 | @@ -1063,7 +1063,7 @@ static size_t secp256k1_pippenger_max_points(secp256k1_scratch *scratch) {
1063 |  #ifdef USE_ENDOMORPHISM
1064 |          entry_size = 2*entry_size;
1065 |  #endif
1066 | -        space_overhead = ((1<<bucket_window) * sizeof(secp256k1_gej) + entry_size + sizeof(struct secp256k1_pippenger_state));
1067 | +        space_overhead = sizeof(secp256k1_gej) << bucket_window + entry_size + sizeof(struct secp256k1_pippenger_state);

practicalswift commented at 9:36 PM on March 30, 2019:

Precedence is hard :-)

        space_overhead = (sizeof(secp256k1_gej) << bucket_window) + entry_size + sizeof(struct secp256k1_pippenger_state);

practicalswift commented at 9:40 PM on March 30, 2019: contributor

FWIW, I believe that Greg's suggestion (with the precedence fix in place) will generate the same binary as 1b8e50570c7227d958989d16fafa40313073a462 under both clang and gcc given -O1.
Use size_t shifts when computing a size_t e6d01e9347

in src/ecmult_impl.h:963 in a064a2ba29 outdated

 959 | @@ -960,7 +960,7 @@ static size_t secp256k1_pippenger_scratch_size(size_t n_points, int bucket_windo
 960 |      size_t entries = n_points + 1;
 961 |  #endif
 962 |      size_t entry_size = sizeof(secp256k1_ge) + sizeof(secp256k1_scalar) + sizeof(struct secp256k1_pippenger_point_state) + (WNAF_SIZE(bucket_window+1)+1)*sizeof(int);
 963 | -    return ((1<<bucket_window) * sizeof(secp256k1_gej) + sizeof(struct secp256k1_pippenger_state) + entries * entry_size);
 964 | +    return sizeof(secp256k1_gej) << bucket_window + sizeof(struct secp256k1_pippenger_state) + entries * entry_size;

practicalswift commented at 9:49 PM on March 30, 2019:

    return (sizeof(secp256k1_gej) << bucket_window) + sizeof(struct secp256k1_pippenger_state) + entries * entry_size;

sipa commented at 10:35 PM on March 30, 2019:

I don't think excessive parentheses help readability.

sipa commented at 11:12 PM on March 30, 2019:

Except in this case they're not actually excessive; they're necessary for correctness. Sorry!

sipa force-pushed on Mar 30, 2019
fanquake cross-referenced this on Mar 31, 2019 from issue Update secp256k1 subtree to latest upstream by sipa
gmaxwell commented at 8:11 AM on March 31, 2019: contributor

ACK. Has someone verified that this silences this particular warning from MSVC (e.g. did we get all the cases?)
sipa commented at 8:19 AM on March 31, 2019: contributor

@gmaxwell Yes, see 15703 in the Bitcoin Core repo; I added (an earlier version of) this change there as an extra commit and it silences AppVeyor.
gmaxwell merged this on Mar 31, 2019
gmaxwell closed this on Mar 31, 2019
gmaxwell referenced this in commit b19c000063 on Mar 31, 2019
real-or-random commented at 10:27 AM on March 31, 2019: contributor
< gmaxwell> sipa: why multiply instead shifting the sizeof?
The multiplication makes the purpose clearer, at least for stupid me.

Whatever, post-merge utACK. :)

Contributors