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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 22, 2025, 13:27 UTC
Message-ID: <d863c518-3246-4752-83f3-469592b1de69@gmail.com>
In-Reply-To: <xmqqplagunnm.fsf@gitster.g>

On 21/10/2025 19:15, Junio C Hamano wrote:

Show 11 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> 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

Show 11 quoted lines

> 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.