Re: [PATCH 3/3] reftable tests: avoid "int" overflow, use "uint64_t"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 13, 2022, 10:04 UTC
Message-ID: <220113.867db4vtk6.gmgdl@evledraar.gmail.com>
In-Reply-To: <Yd8m2YeDZrgSiFzJ@nand.local>

On Wed, Jan 12 2022, Taylor Blau wrote:

Show 39 quoted lines

> On Wed, Jan 12, 2022 at 11:02:05AM -0800, Junio C Hamano wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>> > Am 11.01.22 um 21:18 schrieb Taylor Blau:
>> >> On Tue, Jan 11, 2022 at 09:08:46PM +0100, Johannes Sixt wrote:
>> >>> Am 11.01.22 um 20:41 schrieb Taylor Blau:
>> >>>> On Tue, Jan 11, 2022 at 08:31:47PM +0100, Han-Wen Nienhuys wrote:
>> >>>>> On Tue, Jan 11, 2022 at 8:28 PM Taylor Blau <me@ttaylorr.com> wrote:
>> >>>>>> In any case, you're only setting the lower half of `min` high. Maybe:
>> >>>>>>
>> >>>>>>     uint64_t min = ~0ul;
>> >>>>>
>> >>>>> yeah, that works.
>> >>>>
>> >>>> I'm pretty sure this is OK on 32-bit systems, too, but confirmation from
>> >>>> somebody more confident than I in this area would be welcome :).
>> >>>
>> >>> It does not work on Windows: unsigned long is 32 bits wide. You have to
>> >>> make it
>> >>>
>> >>>    uint64_t min = ~(uint64_t)0;
>> >>
>> >> Perfect; this is exactly what I was looking for. Thanks!
>>
>> That sounds perfect.
>>
>> > Actually, on second thought, UINT64_MAX would be even better.
>>
>> I wouldn't introduce use of UINT64_MAX, which "git grep" does not
>> produce any hits for.
>
>> Unless it is very early in a development cycle, that is, in which
>> case we have enough time to help platforms that are not quite POSIX.
>
> Yep, I agree that avoiding introducing the first instance of UINT64_MAX
> in our tree is worth doing (probably in general, but certainly now that
> we're past even -rc0).
>
> Either `~(uint64_t)0` or `UINTMAX_MAX` would be fine with me.

The reason I left it at 0xffffffff is because the current test clearly doesn't care about using the maximum width of the type, and I was just trying to get rid of the associated compiler warning.

So I'll leave it to Han-Wen to comment on if the "max" being the maximum of the type is actually important here.

As far as what we'd pick to get the maximum type value goes, we should
just prefer whatever we use for that already in that codebase, and we've
got this in a related file there:
    
    reftable/generic.c:     struct reftable_log_record log = {
    reftable/generic.c-             .refname = (char *)name,
    reftable/generic.c-             .update_index = ~((uint64_t)0),
    reftable/generic.c-     };

(Which is what Johannes Sixt independently suggested upthread in <45baffd7-c9f3-cc52-47b4-ea0fee0182a8@kdbg.org>).