Re: Avoid overflow with simplehash

Hi,

On 2023-07-06 13:40:00 -0300, Ranier Vilela wrote:
> I still have doubts about this.
>
> see:
> #include <iostream>
> #include <string>
> #include <limits.h>
>
> #define SH_MAX_SIZE1 (((unsigned long long) 0xFFFFFFFFU) + 1)
> #define SH_MAX_SIZE2 (((unsigned long long) 0xFFFFFFFFU) - 1)
>
> int main()
> {
> unsigned long long max_size1 = SH_MAX_SIZE1;
> unsigned long long max_size2 = SH_MAX_SIZE2;
> unsigned int cur1 = SH_MAX_SIZE1;
> unsigned int cur2 = SH_MAX_SIZE2;
>
> printf("SH_MAX_SIZE1=%llu\n", max_size1);
> printf("SH_MAX_SIZE2=%llu\n", max_size2);
> printf("cur1=%u\n", cur1);
> printf("cur2=%u\n", cur2);
> }
> warning: implicit conversion from 'unsigned long long' to 'unsigned int'
> changes value from 4294967296 to 0 [-Wconstant-conversion]

I don't think we at the moment try to not have implicit-conversion warnings
(nor do we enable them), this would be far from the only place. If we wanted
to here, we'd just need an explicit cast.

> outputs:
> SH_MAX_SIZE1=4294967296
> SH_MAX_SIZE2=4294967294
> cur1=0
> cur2=4294967294
>
> And in the comments we have:
> "Iterate backwards, that allows the current element to be deleted, even
> * if there are backward shifts"
>
> So if an empty element is not found and the *cur* field is set to 0
> (SH_MAX_SIZE -> uint32),

That should never be reachable - which the assert tries to ensure.

> then will it iterate forwards?

No, it'd still iterate backwards, but starting from the wrong place - but
there is no correct place to start iterating from if there is no unused
element.

Greetings,

Andres Freund