Re: [RFC PATCH 02/10] range-diff.c: don't use st_mult() for signed "int"

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 10, 2021, 19:24 UTC
Message-ID: <e881a455-88b5-9c87-03a8-caaee68bb344@gmail.com>
In-Reply-To: <211210.86czm4d3zo.gmgdl@evledraar.gmail.com>

Hi Ævar

On 10/12/2021 12:31, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

> 
> [...] 
> I think I was just chasing butterflies making this intmax_t at all. I
> just submitted a v2, and explained that case in a bit more detail in
> https://lore.kernel.org/git/RFC-cover-v2-0.5-00000000000-20211210T122901Z-avarab@gmail.com
> 
> I *think* it fixes all the cases we plausible run into, i.e. storing the
> "cost" in an "int" was enough, we just needed a size_t as an offset. It
> passes the regression test you noted[3].
> 
> The first thing I tried when hacking on this some months ago (I picked
> these patches up again after running into the segfault again) was this
> s/int/ssize_t/ change.
> 
> I don't think using ssize_t like that is portable, and that we'd need
> something like intmax_t if we needed this in another context.

> Firstly it's not standard C, it's just in POSIX, intmax_t is standard C
> as of C99, which and we have in-tree code that already depends on it
> (and uintmax_t).

I'm not objecting to using intmax_t particularly for code that needs to store negative values other than -1 but we're already using ssize_t in a lot of places so I don't think we need to worry about it not being supported.

> But more importantly it's not "as big as size_t, just signed" in
> POSIX. size_t is "no greater than the width of type long"[1]

The full text is

     The implementation shall support one or more programming
     environments in which the widths of blksize_t, pid_t, size_t,
     ssize_t, and suseconds_t are no greater than the width of type
     long. The names of these programming environments can be obtained
     using the confstr() function or the getconf utility.

so "no greater than the width of type long" applies to ssize_t as well as size_t.

Show 7 quoted lines

> and
> LONG_MAX is at least 2^31-1 [2].
> 
> Whereas ssize_t is not a "signed size_t", but a type that stores
> -1..SSIZE_MAX, and SSIZE_MAX has a minimum value of 2^15-1. I.e. I think
> on that basis some implemenations would make it the same as a "short
> int" under the hood.

The minimum value of SIZE_MAX is 2^16-1[1], I'm not sure you can read much into the width of ssize_t from the minimum value of SSIZE_MAX. If you think about where ssize_t is used as the return value of read() and write() that take a size_t as the number of bytes to read/write then it would be very odd if ssize_t was a different width to size_t.

Best Wishes

Phillip

[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html

Show 12 quoted lines

> On my linux system it's just mapped to the longest available signed
> integer, but that doesn't seem to be a portable assumption.
> 
> 1. https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
> 2. https://pubs.opengroup.org/onlinepubs/009696899/basedefs/limits.h.html
> 
> 3. B.t.w. a thing I ended up ejecting out of this was that I made a
>     "test_commit_bulkier" which is N times faster than "test_commit_bulk",
>     it just makes the same commit N times with the printf-repeating feature
>     and feeds it to fast-import, but the test took so long in any case that
>     I couldn't find a plausible way to get it in-tree).
>