Re: [PATCH 2/8] xdl_change_compact(): clarify code

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Aug 3, 2016, 23:14 UTC
Message-ID: <57715dee-ca73-c1bb-ee79-2813d7873649@alum.mit.edu>
In-Reply-To: <CAGZ79kZk+XW+Bwcx_fvOLVBDse_iUSjEa_K=eJqm4PpTsTAcPA@mail.gmail.com>

On 08/04/2016 12:11 AM, Stefan Beller wrote:

Show 5 quoted lines

> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Write things out a bit longer but less cryptically. Add more comments.
> 
> By less cryptic you mean in Git coding style ;)
> The original author (do we want to cc Davido?) may disagree.

Davide hasn't contributed since 2008 and libxdiff is not being developed, so I didn't think he'd be interested.

Yes, tastes certainly differ. If more people like the old version better, I will gnash my teeth and undo these "clarification" patches. I mean, what's not to like about variable names like "grpsiz" and "ixref"?

Show 17 quoted lines

>> +
>> +                       /*
>> +                        * Are there any blank lines that could appear as the last
>> +                        * line of this group?
>> +                        */
> 
> IIRC this comment is not quite correct as this 'only' counts the number of
> blank lines within the forward shifting section, i.e. in the movable space.
> 
> Later we use it as a boolean indicator (whether or not it is equal to 0)
> to see if we can do better.
> 
> Any other change in code and comments looks good to me, but this stood out
> like a sore thumb. (Probably the old heuristic as a whole stood out, but the
> comment here specifically sounds /wrong/ to me in this place. How can
> a question document a variable? I'd rather expect a question comment
> to ease the understanding of a condition)

I don't understand your objection. A blank line can appear as the last line of the group if and only if it is within the shift range ("movable space") of the group, right? So it seems like our formulations are equivalent.

Since the variable is used as a boolean, it seemed natural to document it by stating the question that the true/false value is the answer to.

If you have a concrete suggestion for a better comment, please let me know.

Michael