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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Aug 10, 2016, 16:39 UTC
Message-ID: <48ca79e2-418b-3c73-e075-808d39a4da9a@alum.mit.edu>
In-Reply-To: <CAGZ79kbyCRDTt4u+Fje819mNZZf3GkZtYVurwOMPXRfXqO-YEw@mail.gmail.com>

On 08/04/2016 01:50 AM, Stefan Beller wrote:

Show 16 quoted lines

> On Wed, Aug 3, 2016 at 4:14 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 08/04/2016 12:11 AM, Stefan Beller wrote:
>>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>>> [...]
>>>> +
>>>> +                       /*
>>>> +                        * 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.
>>> [...]

Thanks for your comments, Stefan.

I realized that the main thing that took me a while to grok when I was reading this code was that blank_lines was really only used as a boolean value, even though it was updated with "+=". That's the main information that I'd like to convey to the reader.

So I decided to change the comment to emphasize this fact (and change it from a question to a statement), and also changed the place that blank_lines is updated to treat it more like a boolean. The latter change also has the advantage of not calling is_blank_line() unnecessarily when blank_lines is already true.

If you have no objections, that is what I will put in v2 of this patch series:

Show 27 quoted lines

> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index de15de2..fde0433 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -460,6 +460,12 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  
>                 do {
>                         groupsize = i - start;
> +
> +                       /*
> +                        * Boolean value that records whether there are any blank
> +                        * lines that could be made to be the last line of this
> +                        * group.
> +                        */
>                         blank_lines = 0;
>  
>                         /*
> @@ -511,7 +517,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>                          * the current change group.
>                          */
>                         while (i < nrec && recs_match(recs, start, i, flags)) {
> -                               blank_lines += is_blank_line(recs, i, flags);
> +                               if (!blank_lines)
> +                                       blank_lines = is_blank_line(recs, i, flags);
>  
>                                 rchg[start++] = 0;
>                                 rchg[i++] = 1;

Michael