D2686: xdiff: add a preprocessing step that trims files

[indygreg (Gregory Szorc)]

indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I'm overall pretty happy with this. I'm requesting just a few minor fixups.
  
  Also, if you (or anyone else for that matter) wanted to spend time to use better variable names and add comments throughout this code, it would be greatly appreciated. I find this code challenging to read because of its almost non-existent documentation.

INLINE COMMENTS

> xprepare.c:169
> +	long plines = 0, pbytes = 0, slines = 0, sbytes = 0, i;
> +	const char *pp1, *pp2, *ps1, *ps2;
> +

Bonus points if you resubmit this with more expressive variable names. Just because xdiff's code is almost impossible to read doesn't mean we should follow suit :)

> xprepare.c:183-193
> +	pp1 = msmall.ptr, pp2 = mlarge.ptr;
> +	for (i = 0; i < msmall.size && *pp1 == *pp2; ++i) {
> +		plines += (*pp1 == '\n');
> +		pp1++, pp2++;
> +	}
> +
> +	ps1 = msmall.ptr + msmall.size - 1, ps2 = mlarge.ptr + mlarge.size - 1;

I'm still showing this as a hot point in the code when compiling with default settings used by Python packaging tools. I suspect we can get better results on typical compiler flags by tweaking things a bit. But we can do that after this lands.

> xprepare.c:199-202
> +		for (i = 0; i <= reserved;) {
> +			pp1--;
> +			i += (*pp1 == '\n');
> +		}

This is clever. But `memrchr()` will be easier to read. Plus I suspect it will be faster.

If you disagree, let's compromise at:

  i = 0;
  while (i <= reserved) {
     pp1--;
     i += (*pp1 == '\n');
  }

There's no sense using a `for` without the 3rd parameter IMO.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2686

To: quark, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel