From: Jeff King <peff@peff.net>
Date: Sun, 14 Aug 2016 12:26:11 GMT
Subject: Re: [PATCH 1/8] xdl_change_compact(): rename some local variables for clarity
Message-ID: <20160814122610.mlg7qnptyngpdggy@sigill.intra.peff.net>
In-Reply-To: <2a36b020-76a5-1ecd-de5c-1d2536847e07@alum.mit.edu>

On Sat, Aug 13, 2016 at 09:38:33PM +0200, Michael Haggerty wrote:

> On 08/04/2016 09:06 AM, Jeff King wrote:
> > On Thu, Aug 04, 2016 at 12:00:29AM +0200, Michael Haggerty wrote:
> > 
> >> * ix -> i
> >> * ixo -> io
> >> * ixs -> start
> >> * grpsiz -> groupsize
> > 
> > After your change, I immediately understand three of them. But what is
> > "io"?
> 
> The (pre-existing) convention in this function is that variable names
> dealing with the "other" file have a trailing "o"; e.g., (xdf, xdfo),
> (rchg, rchgo). There used to also be (i, io), the indexes tracking the
> current line number in the file and the other file. But I renamed "i".

Yeah, after reading the rest of the patches, the "o" prefix sort of
started to make sense.

> At first I was just going to add a comment for variable "io", but in
> trying to figure out its exact semantics I realized that this code is
> still pretty hard to follow. Part of the problem is that "the line in
> the other file corresponding to a line in the to-be-compacted file" is
> not a well-defined concept. In fact it is *groups of lines* that
> correlate with each other. So I totally refactored the function, using a
> 
>     struct group {
>             long start, end;
>     };
> 
> as a kind of a cursor used to iterate through the groups on both sides.
> I think the result is a lot easier to read, and while refactoring I
> found and fixed another bug in the pre-existing code :-O

That sounds like it would be nicer. And bug fixes are always good.

Don't kill yourself polishing up the function names, though (unless you
keep finding bugs. ;) ).

-Peff