Re: [RFC PATCH v5 0/8] rebase-interactive

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mar 26, 2018, 17:01 UTC
Message-ID: <9ca76d31-828d-0b6f-5069-375792c1f55d@jeffhostetler.com>
In-Reply-To: <xmqqzi2ude4w.fsf@gitster-ct.c.googlers.com>

On 3/26/2018 11:56 AM, Junio C Hamano wrote:

Show 34 quoted lines

> Wink Saville <wink@saville.com> writes:
> 
>> json-writer.c:123:38:  error:  format specifies type 'uintmax_t' (aka
>> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
>> long long') [-Werror,-Wformat]
>>
>>          strbuf_addf(&jw->json, ":%"PRIuMAX, value);
>>                                   ~~         ^~~~~
>> json-writer.c:228:37:  error:  format specifies type 'uintmax_t' (aka
>> 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned
>> long long') [-Werror,-Wformat] [0m
>>
>>          strbuf_addf(&jw->json, "%"PRIuMAX, value);
>>                                   ~~         ^~~~~
>> 2 errors generated.
>> make: *** [json-writer.o] Error 1
>> make: *** Waiting for unfinished jobs....
> 
> For whatever reason, our codebase seems to shy away from PRIu64,
> even though there are liberal uses of PRIu32.  Showing the value
> casted to uintmax_t with PRIuMAX seems to be our preferred way to
> say "We cannot say how wide this type is on different platforms, and
> are playing safe by using widest-possible int type" (e.g. showing a
> pid_t value from daemon.c).
> 
> In this codepath, the actual values are specified to be uint64_t, so
> the use of PRIu64 may be OK, but I have to wonder why the codepath
> is not dealing with uintmax_t in the first place.  When even larger
> than present archs are prevalent in N years and 64-bit starts to
> feel a tad small (like we feel for 16-bit ints these days), it will
> feel a bit silly to have a subsystem that is limited to such a
> "fixed and a tad small these days" types and pretend it to be be a
> generic seriealizer, I suspect.
>

I defined that routine to take a uint64_t because I wanted to pass a nanosecond value received from getnanotime() and that's what it returns.

My preference would be to change the PRIuMAX to PRIu64, but there aren't any other references in the code to that symbol and I didn't want to start a new trend here.

I am concerned that the above compiler error message says that uintmax_t is defined as an "unsigned long" (which is defined as *at least* 32 bits, but not necessarily 64. But a uint64_t is defined as a "unsigned long long" and guaranteed as a 64 bit value.

So while I'm not really worried about 128 bit integers right now, I'm more concerned about 32 bit compilers truncating that value without any warnings.

Jeff