Re: [PATCH v2 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char
- From
Ezekiel Newren <ezekielnewren@gmail.com>
- Date
- Nov 6, 2025, 23:13 UTC
- Message-ID
- <CAH=ZcbA7d5Z7d=VT2_o=+M8pYrGzO7TgAaLisk2k0p7CuQuSPQ@mail.gmail.com>
- In-Reply-To
- <299e25d6-caaf-4672-8160-53fdafe96134@gmail.com>
On Thu, Nov 6, 2025 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 12 quoted lines
> > Hi Ezekiel > > On 29/10/2025 22:19, Ezekiel Newren via GitGitGadget wrote: > > From: Ezekiel Newren <ezekielnewren@gmail.com> > > > > Rust uses u8 to refer to bytes in memory. Since xrecord_t.ptr is also > > referring to bytes in memory, rather than Unicode code points, use > > uint8_t instead of char. > > The reference to unicode code points here still makes no sense to me. I > thought the reason for the conversion was to match rust's u8.
It is to match Rust's u8 type, but I was also trying to convey that ptr is referring to bytes and not characters _because_ xdiff performs textual differences. It's not spelled out anywhere in Xdiff that it does or doesn't take Unicode into consideration. Would comparing Unicode code points change how Xdiff behaves? Should it behave differently? I don't know. My understanding is that whether the bytes are utf-8, utf-16le, utf-16be, or some other encoding of Unicode. Xdiff doesn't care and treats the lines in a file as raw byte strings.
There's also the question of "Should the Rust side of Xdiff treat lines in a file as &[u8] or &str?" The reason why this matters is because in order to get a &str from &[u8] in Rust you need to call a function like:
```
let raw_bytes = b"abc\n";
let result = std::str::from_utf8(raw_bytes);
if let Ok(line) = result {
// do something
}
```What happens if it's not utf8 encoded? What if it's malformed utf8? To avoid these problems I only use &[u8] in xdiff and perform differences on raw byte strings rather than considering Unicode at all like how Xdiff already does.
Does that explain my comment about Unicode or does it still seem out of place to you? I can remove the mention of Unicode from the commit message if this still doesn't make any sense to you.
Show 9 quoted lines
> > Every usage of this field was inspected and cast to char*, or similar, > > to avoid signedness warnings/errors from the compiler. Casting was used > > so that the whole of xdiff doesn't need to be refactored in order to > > change the type of this field. > > Thanks for adding this. Having played a little with changing some > function parameters to avoid adding these casts I agree this patch is a > good place to stop as the number of changes required quickly spiraled > out of control.
I'm not excited about the casts either, but these 2 structs are fundamental to how Xdiff passes data around, and so they need to be FFI friendly. I don't plan on converting other structs or function signatures in Xdiff unless I really have to.