Skip to content

Xdiff cleanup part2

Ezekiel Newren via GitGitGadget git 5 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v5 Latest10 patches 3 repliesIn review
  2. v410 patches 10 repliesSuperseded
  3. v310 patches 12 repliesSuperseded
Earlier versions · 2
VersionPostedPatchesRepliesStatus
v210 patches12 repliesSuperseded
v19 patches27 repliesSuperseded

Changes in v5 from the cover letter

* Remove the non-word 'signless', and rephrase that paragraph in unambiguous-types.adoc
* Cast to char in xdiffi.c:get_indent() rather than changing the local variable to uint8_t

Patches in v5 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/10doc: define unambiguous type mappings across C and RustDocumentation/Makefile Documentation/technical/meson.build Documentation/technical/unambiguous-types.adoc+226 -02 replies
2/10xdiff: use ptrdiff_t for dstart/dendxdiff/xtypes.h+1 -10 replies
3/10xdiff: make xrecord_t.ptr a uint8_t instead of charxdiff/xdiffi.c xdiff/xemit.c xdiff/xmerge.c +4 more+21 -210 replies
4/10xdiff: use size_t for xrecord_t.sizexdiff/xdiffi.c xdiff/xemit.c xdiff/xmerge.c +2 more+19 -200 replies
5/10xdiff: use unambiguous types in xdl_hash_record()xdiff-interface.c xdiff/xprepare.c xdiff/xutils.c +1 more+21 -210 replies
6/10xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hashxdiff/xdiffi.c xdiff/xhistogram.c xdiff/xpatience.c +2 more+21 -200 replies
7/10xdiff: make xdfile_t.nrec a size_t instead of longxdiff/xdiffi.c xdiff/xemit.c xdiff/xmerge.c +3 more+26 -260 replies
8/10xdiff: make xdfile_t.nreff a size_t instead of longxdiff/xprepare.c xdiff/xtypes.h+8 -80 replies
9/10xdiff: change rindex from long to size_t in xdfile_txdiff/xtypes.h+1 -10 replies
10/10xdiff: rename rindex -> reference_indexxdiff/xdiffi.c xdiff/xprepare.c xdiff/xtypes.h+9 -90 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v5 yet.

Read the v5 thread

Cover letter [PATCH v5 00/10] Xdiff cleanup part2

Changes in v5:

* Remove the non-word 'signless', and rephrase that paragraph in unambiguous-types.adoc
* Cast to char in xdiffi.c:get_indent() rather than changing the local variable to uint8_t

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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2070/ezekielnewren/xdiff_cleanup_part2-v5
Pull-Request: https://github.com/git/git/pull/2070

Range-diff vs v4:

1: af732beb69 ! 1: 8b56bf1172 doc: define unambiguous type mappings across C and Rust
@@ Documentation/technical/unambiguous-types.adoc (new)
+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.
++The C language leaves the signedness of char implementation defined. Because ++our developer build enables -Wsign-compare, comparison of a value of char ++type with either signed or unsigned integers may trigger warnings from the ++compiler.
+
+Note: Rust's char type is an unsigned 32-bit integer that is used to describe +Unicode code points.

  2:  b60a03eb31 =  2:  c4193d11f5 xdiff: use ptrdiff_t for dstart/dend
  3:  042fbb11d0 !  3:  dd76d4f586 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.

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.
+ cast to char* or similar.

Places where casting was unnecessary:
xemit.c:156
@@ xdiff/xdiffi.c: static int get_indent(xrecord_t *rec)

for (i = 0; i < rec->size; i++) {
- char c = rec->ptr[i];
-+ uint8_t c = rec->ptr[i];
++ char c = (char) rec->ptr[i];

if (!XDL_ISSPACE(c))
return ret;
4: c103fa6bea ! 4: 11cec1d2ec xdiff: use size_t for xrecord_t.size
@@ xdiff/xdiffi.c: static int recs_match(xrecord_t *rec1, xrecord_t *rec2)

- for (i = 0; i < rec->size; i++) {
+ for (size_t i = 0; i < rec->size; i++) {
- uint8_t c = rec->ptr[i];
+ char c = (char) rec->ptr[i];

if (!XDL_ISSPACE(c))
@@ xdiff/xdiffi.c: static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)

  5:  2ee9a74653 =  5:  6f267360b7 xdiff: use unambiguous types in xdl_hash_record()
  6:  f044274bd5 =  6:  78af0f16f4 xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash
  7:  f7a3731d94 =  7:  5c19f9ded3 xdiff: make xdfile_t.nrec a size_t instead of long
  8:  93f84ae72e =  8:  d1f498edb1 xdiff: make xdfile_t.nreff a size_t instead of long
  9:  39369becc8 =  9:  bc4941c146 xdiff: change rindex from long to size_t in xdfile_t
 10:  950d1e6193 = 10:  dcc9d6bfaf xdiff: rename rindex -> reference_index
Signature · 1 line
gitgitgadget