Web lists-archives.com

Re: [PATCH] xdiff: improve trimming preprocessing




On Tue, Mar 6, 2018 at 6:05 PM, Jun Wu <quark@xxxxxx> wrote:
> 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@xxxxxx> 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.

>> > +       /* 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.