Re: [PATCH] xdiff: improve trimming preprocessing

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 7, 2018, 09:36 UTC
Message-ID: <CAPig+cToTv063z-HB=8Y9LkpbqQaKCVJG_nU0Qd7O9ZdJY-p6g@mail.gmail.com>
In-Reply-To: <1520370729-sup-3241@x1c>

On Tue, Mar 6, 2018 at 6:05 PM, Jun Wu <quark@fb.com> wrote:

Show 8 quoted lines

> Excerpts from Eric Sunshine's message of 2018-03-06 14:23:46 -0500:
>> On Tue, Mar 6, 2018 at 6:53 AM, Jun Wu <quark@fb.com> wrote:
>> > +  printf "x%.0s" {1..934} >>d # pad common suffix to 1024 bytes
>>
>> The expression {x..y} is not portable to non-POSIX shells.
>
> Is there a recommended way to generate a repetitive string?
> Maybe `seq 1000 | sed 's/.*/x/'` ?

That seems reasonable, although you'd want to use the test suite's more portable test_seq rather than seq.

Show 16 quoted lines

>> > +       /* prefix - need line count for xecfg->ptrimmed */
>> > +       for (i = 0; ++i < smaller && *ap == *bp;) {
>> > +               lines += (*ap == '\n');
>> > +               ap++, bp++;
>>
>> Is there a good reason for not placing 'ap++, bp++' directly in the 'for'?
>
> "lines += (*ap == '\n');" needs the "ap" before adding. Alternatives are
>
>     for (i = 0; ++i < smaller && *ap == *bp; ) /* 1 */
>         lines += (*ap++, *bp++) == '\n';
>
>     for (i = 0; ++i < smaller && *ap == *bp; ap++, bp++) /* 2 */
>         lines += (*(ap - 1) == '\n');
>
> Maybe will pick /* 1 */ to make the code shorter.

I was thinking specifically about #2 when asking the question. The reason I asked is that, as one reading the code, the not-quite-idiomatic loop made me stop and wonder if something special was going on which wasn't obvious at a glance.

And, apparently, I'm still wondering since I'm not groking what you mean by:

    "lines += (*ap == '\n');" needs the "ap" before adding

It's quite possible that I'm being dense and overlooking something blindingly obvious.