From: Phillip Wood <phillip.wood123@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: Ezekiel Newren via GitGitGadget <gitgitgadget@gmail•com>,
git@vger•kernel.org, Ezekiel Newren <ezekielnewren@gmail•com>,
Patrick Steinhardt <ps@pks•im>
Subject: Re: [PATCH 2/9] xdiff: make xrecord_t.ptr a uint8_t instead of char
Date: Wed, 22 Oct 2025 14:27:09 +0100 [thread overview]
Message-ID: <d863c518-3246-4752-83f3-469592b1de69@gmail.com> (raw)
In-Reply-To: <xmqqplagunnm.fsf@gitster.g>
On 21/10/2025 19:15, Junio C Hamano wrote:
> 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
> 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.
next prev parent reply other threads:[~2025-10-22 13:27 UTC|newest]
Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-15 21:18 [PATCH 0/9] Xdiff cleanup part2 Ezekiel Newren via GitGitGadget
2025-10-15 21:18 ` [PATCH 1/9] xdiff: use ssize_t for dstart/dend, make them last in xdfile_t Ezekiel Newren via GitGitGadget
2025-10-21 11:32 ` Phillip Wood
2025-10-21 17:18 ` Junio C Hamano
2025-10-22 21:07 ` Ezekiel Newren
2025-10-22 21:38 ` Junio C Hamano
2025-10-22 21:51 ` Ezekiel Newren
2025-10-15 21:18 ` [PATCH 2/9] xdiff: make xrecord_t.ptr a uint8_t instead of char Ezekiel Newren via GitGitGadget
2025-10-16 21:51 ` Kristoffer Haugsbakk
2025-10-21 8:33 ` Patrick Steinhardt
2025-10-22 21:12 ` Ezekiel Newren
2025-10-21 13:13 ` Phillip Wood
2025-10-21 18:15 ` Junio C Hamano
2025-10-22 13:27 ` Phillip Wood [this message]
2025-10-22 20:55 ` Ezekiel Newren
2025-10-15 21:18 ` [PATCH 3/9] xdiff: use size_t for xrecord_t.size Ezekiel Newren via GitGitGadget
2025-10-15 21:18 ` [PATCH 4/9] xdiff: use unambiguous types in xdl_hash_record() Ezekiel Newren via GitGitGadget
2025-10-21 8:33 ` Patrick Steinhardt
2025-10-22 21:20 ` Ezekiel Newren
2025-10-23 5:49 ` Patrick Steinhardt
2025-10-15 21:18 ` [PATCH 5/9] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Ezekiel Newren via GitGitGadget
2025-10-20 23:29 ` Ezekiel Newren
2025-10-21 5:10 ` Junio C Hamano
2025-10-21 8:33 ` Patrick Steinhardt
2025-10-21 10:03 ` Phillip Wood
2025-10-21 11:16 ` Chris Torek
2025-10-22 21:31 ` Ezekiel Newren
2025-10-15 21:18 ` [PATCH 6/9] xdiff: make xdfile_t.nrec a size_t instead of long Ezekiel Newren via GitGitGadget
2025-10-15 21:18 ` [PATCH 7/9] xdiff: make xdfile_t.nreff " Ezekiel Newren via GitGitGadget
2025-10-15 21:18 ` [PATCH 8/9] xdiff: change rindex from long to size_t in xdfile_t Ezekiel Newren via GitGitGadget
2025-10-21 8:34 ` Patrick Steinhardt
2025-10-22 22:14 ` Ezekiel Newren
2025-10-23 5:49 ` Patrick Steinhardt
2025-10-15 21:18 ` [PATCH 9/9] xdiff: rename rindex -> reference_index Ezekiel Newren via GitGitGadget
2025-10-15 21:28 ` [PATCH 0/9] Xdiff cleanup part2 Junio C Hamano
2025-10-21 13:28 ` Phillip Wood
2025-10-21 13:41 ` Junio C Hamano
2025-10-29 22:19 ` [PATCH v2 00/10] " Ezekiel Newren via GitGitGadget
2025-10-29 22:19 ` [PATCH v2 01/10] doc: define unambiguous type mappings across C and Rust Ezekiel Newren via GitGitGadget
2025-11-06 9:55 ` Phillip Wood
2025-11-06 22:52 ` Ezekiel Newren
2025-11-09 14:14 ` Phillip Wood
2025-10-29 22:19 ` [PATCH v2 02/10] xdiff: use ssize_t for dstart/dend, make them last in xdfile_t Ezekiel Newren via GitGitGadget
2025-11-06 9:55 ` Phillip Wood
2025-11-06 22:56 ` Ezekiel Newren
2025-10-29 22:19 ` [PATCH v2 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char Ezekiel Newren via GitGitGadget
2025-11-06 10:49 ` Phillip Wood
2025-11-06 23:13 ` Ezekiel Newren
2025-11-06 10:55 ` Phillip Wood
2025-11-06 23:14 ` Ezekiel Newren
2025-10-29 22:19 ` [PATCH v2 04/10] xdiff: use size_t for xrecord_t.size Ezekiel Newren via GitGitGadget
2025-10-29 22:19 ` [PATCH v2 05/10] xdiff: use unambiguous types in xdl_hash_record() Ezekiel Newren via GitGitGadget
2025-10-29 22:19 ` [PATCH v2 06/10] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Ezekiel Newren via GitGitGadget
2025-11-06 11:00 ` Phillip Wood
2025-11-06 23:20 ` Ezekiel Newren
2025-10-29 22:19 ` [PATCH v2 07/10] xdiff: make xdfile_t.nrec a size_t instead of long Ezekiel Newren via GitGitGadget
2025-10-29 22:19 ` [PATCH v2 08/10] xdiff: make xdfile_t.nreff " Ezekiel Newren via GitGitGadget
2025-10-29 22:19 ` [PATCH v2 09/10] xdiff: change rindex from long to size_t in xdfile_t Ezekiel Newren via GitGitGadget
2025-10-29 22:19 ` [PATCH v2 10/10] xdiff: rename rindex -> reference_index Ezekiel Newren via GitGitGadget
2025-10-30 14:26 ` [PATCH v2 00/10] Xdiff cleanup part2 Junio C Hamano
2025-11-11 19:42 ` [PATCH v3 " Ezekiel Newren via GitGitGadget
2025-11-11 19:42 ` [PATCH v3 01/10] doc: define unambiguous type mappings across C and Rust Ezekiel Newren via GitGitGadget
2025-11-11 20:52 ` Junio C Hamano
2025-11-11 21:05 ` Junio C Hamano
2025-11-11 19:42 ` [PATCH v3 02/10] xdiff: use ptrdiff_t for dstart/dend Ezekiel Newren via GitGitGadget
2025-11-11 22:23 ` Junio C Hamano
2025-11-11 19:42 ` [PATCH v3 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char Ezekiel Newren via GitGitGadget
2025-11-11 22:53 ` Junio C Hamano
2025-11-11 19:42 ` [PATCH v3 04/10] xdiff: use size_t for xrecord_t.size Ezekiel Newren via GitGitGadget
2025-11-11 23:08 ` Junio C Hamano
2025-11-14 6:02 ` Ezekiel Newren
2025-11-14 16:31 ` Junio C Hamano
2025-11-11 19:42 ` [PATCH v3 05/10] xdiff: use unambiguous types in xdl_hash_record() Ezekiel Newren via GitGitGadget
2025-11-11 19:42 ` [PATCH v3 06/10] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Ezekiel Newren via GitGitGadget
2025-11-11 23:21 ` Junio C Hamano
2025-11-14 5:41 ` Ezekiel Newren
2025-11-14 20:06 ` Junio C Hamano
2025-11-11 19:42 ` [PATCH v3 07/10] xdiff: make xdfile_t.nrec a size_t instead of long Ezekiel Newren via GitGitGadget
2025-11-11 19:42 ` [PATCH v3 08/10] xdiff: make xdfile_t.nreff " Ezekiel Newren via GitGitGadget
2025-11-11 19:42 ` [PATCH v3 09/10] xdiff: change rindex from long to size_t in xdfile_t Ezekiel Newren via GitGitGadget
2025-11-11 19:42 ` [PATCH v3 10/10] xdiff: rename rindex -> reference_index Ezekiel Newren via GitGitGadget
2025-11-11 23:40 ` [PATCH v3 00/10] Xdiff cleanup part2 Junio C Hamano
2025-11-14 5:52 ` Ezekiel Newren
2025-11-14 22:36 ` [PATCH v4 " Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 01/10] doc: define unambiguous type mappings across C and Rust Ezekiel Newren via GitGitGadget
2025-11-15 3:06 ` Ramsay Jones
2025-11-15 3:41 ` Ben Knoble
2025-11-15 14:55 ` Ramsay Jones
2025-11-15 16:42 ` Junio C Hamano
2025-11-15 16:59 ` D. Ben Knoble
2025-11-15 20:03 ` Junio C Hamano
2025-11-17 1:20 ` Junio C Hamano
2025-11-17 2:08 ` Ramsay Jones
2025-11-14 22:36 ` [PATCH v4 02/10] xdiff: use ptrdiff_t for dstart/dend Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char Ezekiel Newren via GitGitGadget
2025-11-15 8:26 ` Junio C Hamano
2025-11-18 20:55 ` Ezekiel Newren
2025-11-14 22:36 ` [PATCH v4 04/10] xdiff: use size_t for xrecord_t.size Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 05/10] xdiff: use unambiguous types in xdl_hash_record() Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 06/10] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 07/10] xdiff: make xdfile_t.nrec a size_t instead of long Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 08/10] xdiff: make xdfile_t.nreff " Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 09/10] xdiff: change rindex from long to size_t in xdfile_t Ezekiel Newren via GitGitGadget
2025-11-14 22:36 ` [PATCH v4 10/10] xdiff: rename rindex -> reference_index Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 00/10] Xdiff cleanup part2 Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 01/10] doc: define unambiguous type mappings across C and Rust Ezekiel Newren via GitGitGadget
2025-11-18 23:46 ` Ramsay Jones
2025-11-19 4:14 ` Junio C Hamano
2025-11-18 22:34 ` [PATCH v5 02/10] xdiff: use ptrdiff_t for dstart/dend Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 03/10] xdiff: make xrecord_t.ptr a uint8_t instead of char Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 04/10] xdiff: use size_t for xrecord_t.size Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 05/10] xdiff: use unambiguous types in xdl_hash_record() Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 06/10] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 07/10] xdiff: make xdfile_t.nrec a size_t instead of long Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 08/10] xdiff: make xdfile_t.nreff " Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 09/10] xdiff: change rindex from long to size_t in xdfile_t Ezekiel Newren via GitGitGadget
2025-11-18 22:34 ` [PATCH v5 10/10] xdiff: rename rindex -> reference_index Ezekiel Newren via GitGitGadget
2025-11-18 23:11 ` [PATCH v5 00/10] Xdiff cleanup part2 Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d863c518-3246-4752-83f3-469592b1de69@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=ezekielnewren@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox