Re: [PATCH 2/8] xdl_change_compact(): clarify code
From: Michael Haggerty <hidden>
Date: 2016-08-10 18:47:46
On 08/04/2016 01:50 AM, Stefan Beller wrote:
On Wed, Aug 3, 2016 at 4:14 PM, Michael Haggerty [off-list ref] wrote:quoted
On 08/04/2016 12:11 AM, Stefan Beller wrote:quoted
On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty [off-list ref] wrote:quoted
[...] + + /* + * 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:
quoted hunk ↗ jump to hunk
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