[PATCH v4 00/10] Xdiff cleanup part2

["Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com>]

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 <ezekielnewren@gmail.com>
     @@ 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 <ezekielnewren@gmail.com>
      
       ## 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 <ezekielnewren@gmail.com>
      
  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 <ezekielnewren@gmail.com>
  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