Subject: Re: [PATCH 2/9] xdiff: make xrecord_t.ptr a uint8_t instead of char

From: Phillip Wood <[email-masked]>
Date: Wed, 22 Oct 2025 14:27:09 +0100

On 21/10/2025 19:15, Junio C Hamano wrote:
> Phillip Wood <[email-masked]> writes:
> 
>> It C "char" never refers to a unicode code point so I don't follow the
>> reasoning here. Isn't the reason you want to change from "char" to
>> "uint8_t" to match rust? Given "char" and "uint8_t" are the same width
>> why can't we use "char" in the C struct and "u8" in the rust struct as
>> the two structs would still have the same layout?
> 
> And forcing u8 makes sure both sides of the ffi agrees on the
> signedness (C "char"'s signedness is implementation defined),
> which is a good thing.

That's true and ignoring the signedness would be hacky but I'm not sure 
it matters in practice. Both C and rust would use the same bit patterns 
for "abc" and b"abc\0" and in general C plays fast and loose with the 
signedness of variables all over the place. The trade off for respecting 
the signedness is that we either have casts all over the place or 
massive churn converting the rest of the code to use uint8_t. This 
problem isn't limited to xdiff, it will be true wherever we share 
bytestrings such as the contents of objects between C and rust as we 
tend to use char rather than uint8_t in our code.

Thanks

Phillip

> I 100% agree that being honest about the motivation to sell this
> change would be a good thing to do here.  I do not think "in this
> series, I want to match the types used at the interface to be of
> Rust's" is a position to be ashamed of ;-)
> 
>> I agree with Patrick's comments on this patch - it would be nice to know
>> how you decided where to add casts. Given that rust is going to be
>> optional for at least a year we should take care to leave the C code in
>> good shape with a minimum number of casts.
> 
> Thanks.