From: Ezekiel Newren via GitGitGadget Date: Fri, 14 Nov 2025 22:36:46 GMT Subject: [PATCH v4 00/10] Xdiff cleanup part2 Message-ID: In-Reply-To: Changes in v4: * Update documentation to not mention Unicode except once * Don't move dstart/dend with in the xdfile_t struct * Rephrase justification on changing xrecord_t.ptr's type Changes in v3: * Address comments about commit messages and documentation * Add unambiguous-types.adoc to Makefile and Meson * Use markdown style to avoid asciidoc issues Changes in v2: * Added documentation about unambiguous types and FFI * Addressed comments on the mailing list Original cover letter below: ============================ Maintainer note: This patch series builds on top of en/xdiff-cleanup and am/xdiff-hash-tweak (both of which are now in master). The primary goal of this patch series is to convert every field's type in xrecord_t and xdfile_t to be unambiguous, in preparation to make it more Rust FFI friendly. Additionally the ha field in xrecord_t is split into line_hash and minimal_perfect hash. The order of some of the fields has changed as called out by the commit messages. Before: typedef struct s_xrecord { char const *ptr; long size; unsigned long ha; } xrecord_t; typedef struct s_xdfile { xrecord_t *recs; long nrec; long dstart, dend; bool *changed; long *rindex; long nreff; } xdfile_t; After part 2 typedef struct s_xrecord { uint8_t const *ptr; size_t size; uint64_t line_hash; size_t minimal_perfect_hash; } xrecord_t; typedef struct s_xdfile { xrecord_t *recs; size_t nrec; ptrdiff_t dstart, dend; bool *changed; size_t *reference_index; size_t nreff; } xdfile_t; Ezekiel Newren (10): doc: define unambiguous type mappings across C and Rust xdiff: use ptrdiff_t for dstart/dend xdiff: make xrecord_t.ptr a uint8_t instead of char xdiff: use size_t for xrecord_t.size xdiff: use unambiguous types in xdl_hash_record() xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash xdiff: make xdfile_t.nrec a size_t instead of long xdiff: make xdfile_t.nreff a size_t instead of long xdiff: change rindex from long to size_t in xdfile_t xdiff: rename rindex -> reference_index Documentation/Makefile | 1 + Documentation/technical/meson.build | 1 + .../technical/unambiguous-types.adoc | 224 ++++++++++++++++++ xdiff-interface.c | 2 +- xdiff/xdiffi.c | 29 ++- xdiff/xemit.c | 28 +-- xdiff/xhistogram.c | 4 +- xdiff/xmerge.c | 30 +-- xdiff/xpatience.c | 14 +- xdiff/xprepare.c | 60 ++--- xdiff/xtypes.h | 15 +- xdiff/xutils.c | 32 +-- xdiff/xutils.h | 6 +- 13 files changed, 336 insertions(+), 110 deletions(-) create mode 100644 Documentation/technical/unambiguous-types.adoc base-commit: a99f379adf116d53eb11957af5bab5214915f91d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2070%2Fezekielnewren%2Fxdiff_cleanup_part2-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2070/ezekielnewren/xdiff_cleanup_part2-v4 Pull-Request: https://github.com/git/git/pull/2070 Range-diff vs v3: 1: e5d084d340 ! 1: af732beb69 doc: define unambiguous type mappings across C and Rust @@ Metadata ## Commit message ## doc: define unambiguous type mappings across C and Rust - Document other nuances with crossing the FFI boundary. Other language + Document other nuances when crossing the FFI boundary. Other language mappings may be added in the future. Signed-off-by: Ezekiel Newren @@ Documentation/technical/unambiguous-types.adoc (new) + +This is where C and Rust don't have a clean one-to-one mapping. + ++A C `char` and a Rust `u8` share the same bit width, so any C struct containing ++a `char` will have the same size as the corresponding Rust struct using `u8`. ++In that sense, such structs are safe to pass over the FFI boundary, because ++their fields will be laid out identically. However, beyond bit width, C `char` ++has additional semantics and platform-dependent behavior that can cause ++problems, as discussed below. ++ +C comparison problem: While the sign of `char` is implementation defined, it's +also signless (neither signed nor unsigned). When building with +`make DEVELOPER=1` it will complain about a "differ in signedness" when `char` +is compared with `uint8_t` or `int8_t`. + -+Rust's `char` type is an unsigned 32-bit integer that is used to describe -+Unicode code points. Even though a C `char` is the same width as `u8`, `char` -+should be converted to u8 where it is describing bytes in memory. If a C -+`char` is not describing bytes, then it should be converted to a more accurate -+unambiguous type. The reason for mentioning Unicode here is because of how &str -+is defined in Rust and how to create a &str from &[u8]. Rust assumes that &str -+is a correctly encoded utf-8 string, i.e. text in memory. Where as a C `char` -+makes no assumption about the bytes that it is representing. -+ -+``` -+let raw_bytes = b"abc\n"; -+let result = std::str::from_utf8(raw_bytes); -+if let Ok(line) = result { -+ // do something with text -+} -+``` -+ -+While you could specify `char` in the C code and `u8` in Rust code, it's not as -+clear what the appropriate type is, but it would work across the FFI boundary. -+However, the bigger problem comes from code generation tools like cbindgen and -+bindgen. When cbindgen sees u8 in Rust it will generate uint8_t on the C side -+which will cause differ in signedness warnings/errors. Similarly if bindgen -+sees `char` on the C side it will generate `std::ffi::c_char` which has its own -+problems. ++Note: Rust's `char` type is an unsigned 32-bit integer that is used to describe ++Unicode code points. + +=== Notes +^1^ This is only true if stdbool.h (or equivalent) is used. + 2: 52e3f589b1 ! 2: b60a03eb31 xdiff: use ptrdiff_t for dstart/dend @@ Commit message ptrdiff_t is appropriate for dstart and dend because they both describe positive or negative offsets relative to a pointer. - A future patch will move these fields to a different struct. Moving - them to the end of xdfile_t now, means the field order of xdfile_t will - be disturbed less. - Signed-off-by: Ezekiel Newren ## xdiff/xtypes.h ## @@ xdiff/xtypes.h: typedef struct s_xrecord { xrecord_t *recs; long nrec; - long dstart, dend; ++ ptrdiff_t dstart, dend; bool *changed; long *rindex; long nreff; -+ ptrdiff_t dstart, dend; - } xdfile_t; - - typedef struct s_xdfenv { 3: 83e7bf180a ! 3: 042fbb11d0 xdiff: make xrecord_t.ptr a uint8_t instead of char @@ Commit message Make xrecord_t.ptr uint8_t because it's referring to bytes in memory. - 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. + In order to avoid a refactor avalanche, many uses of this field were + cast to char* or similar. One exception is in get_indent() where the + local variable `char c` was changed to `uint8_t c`. + + Places where casting was unnecessary: + xemit.c:156 + xmerge.c:124 + xmerge.c:127 + xmerge.c:164 + xmerge.c:169 + xmerge.c:172 + xmerge.c:178 Signed-off-by: Ezekiel Newren 4: da2b80ea0b = 4: c103fa6bea xdiff: use size_t for xrecord_t.size 5: c6ba630ac5 = 5: 2ee9a74653 xdiff: use unambiguous types in xdl_hash_record() 6: 3834ea8f9b ! 6: f044274bd5 xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash @@ Commit message field. It comes from the classifier's general-purpose hash table, which assigns each line a unique and minimal hash across the two files. A size_t is used here because it's meant to be used to - index an array. This also this avoids ` as usize` casts on the Rust + index an array. This also avoids ` as usize` casts on the Rust side when using it to index a slice. Signed-off-by: Ezekiel Newren 7: e2a2c7530c ! 7: f7a3731d94 xdiff: make xdfile_t.nrec a size_t instead of long @@ xdiff/xtypes.h: typedef struct s_xrecord { xrecord_t *recs; - long nrec; + size_t nrec; + ptrdiff_t dstart, dend; bool *changed; long *rindex; - long nreff; 8: 31cd2a1aa4 ! 8: 93f84ae72e xdiff: make xdfile_t.nreff a size_t instead of long @@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t * ## xdiff/xtypes.h ## @@ xdiff/xtypes.h: typedef struct s_xdfile { - size_t nrec; + ptrdiff_t dstart, dend; bool *changed; long *rindex; - long nreff; + size_t nreff; - ptrdiff_t dstart, dend; } xdfile_t; + typedef struct s_xdfenv { 9: aee0d3958b ! 9: 39369becc8 xdiff: change rindex from long to size_t in xdfile_t @@ Commit message ## xdiff/xtypes.h ## @@ xdiff/xtypes.h: typedef struct s_xdfile { - xrecord_t *recs; size_t nrec; + ptrdiff_t dstart, dend; bool *changed; - long *rindex; + size_t *rindex; size_t nreff; - ptrdiff_t dstart, dend; } xdfile_t; + 10: 75c26fe160 ! 10: 950d1e6193 xdiff: rename rindex -> reference_index @@ xdiff/xprepare.c: static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t * ## xdiff/xtypes.h ## @@ xdiff/xtypes.h: typedef struct s_xdfile { - xrecord_t *recs; size_t nrec; + ptrdiff_t dstart, dend; bool *changed; - size_t *rindex; + size_t *reference_index; size_t nreff; - ptrdiff_t dstart, dend; } xdfile_t; + -- gitgitgadget