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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to