Re: [PATCH 2/2] diff --word-diff: use non-whitespace regex by default

From: Tay Ray Chuan <rctay89@gmail.com>
Date: Jan 20, 2012, 01:14 UTC
Message-ID: <CALUzUxq8dsc-rO6fVcOEvkaAtuJv7vHRYoUS++3D1nsJsyCrbw@mail.gmail.com>
In-Reply-To: <87bopzofir.fsf@thomas.inf.ethz.ch>

On Thu, Jan 19, 2012 at 11:53 PM, Thomas Rast <trast@student.ethz.ch> wrote:

Show 6 quoted lines

>[snip]
> Under [^[:space:]]+ neither of the examples would work.  Actually,
> [^[:space:]]+ is the same as today's default, the [^[:space:]]* I
> mentioned later is (strictly speaking) broken as it allows for a
> 0-length match.  (It doesn't really matter because IIRC the engine
> ignores 0-length words.)

My bad.

Show 28 quoted lines

>[snip]
> I tried measuring it across a few commits, but it mostly gets drowned
> out by the diff effort.  For a commit with stat
>
>  exercises/cgal/cover/cover.cpp  |    5 +-
>  exercises/cgal/cover/cover.in1  |27014 +++++++++++++++-----
>  exercises/cgal/cover/cover.in2  |48996 +++++++++++++++++++++++------------
>  exercises/cgal/cover/cover.in3  |55041 +++++++++++++++++++++++++--------------
>  exercises/cgal/cover/cover.in4  |47600 ++++++++++++++++++++--------------
>  exercises/cgal/cover/cover.int  |43491 ++++++++++++++++++++++---------
>  exercises/cgal/cover/cover.out1 |   53 +-
>  exercises/cgal/cover/cover.out2 |   24 +-
>  exercises/cgal/cover/cover.out3 |   11 +-
>  exercises/cgal/cover/cover.out4 |    2 +-
>  exercises/cgal/cover/cover.outt |   23 +-
>  exercises/cgal/cover/gen        |   39 +-
>  exercises/cgal/cover/gen-1.cpp  |    4 +-
>  exercises/cgal/cover/gen-2.cpp  |    6 +-
>  exercises/cgal/cover/gen-3.cpp  |    6 +-
>
> (sorry, can't share as those testcases are secret) I get best-of-5
> timings
>
>  --word-diff-regex='[^[:space:]]+'    0:07.50real 7.40user 0.07system
>  --word-diff                          0:07.47real 7.41user 0.03system
>
> In conclusion, "meh".  I think ripping out the isspace() part would make
> for a nice code reduction.

Thanks for the numbers. Well, that agrees with the intuition that regex is slower than isspace(), since you have run it through the regex engine.

Show 37 quoted lines

>>> and your proposal is equivalent to
>>>
>>>  [^[:space:]]|UTF_8_GUARD
>>>
>>> I think there is a case to be made for a default of
>>>
>>>  [^[:space:]]|([[:alnum:]]|UTF_8_GUARD)+
>>>
>>> or some such.  There's a lot of bikeshedding lurking in the (non)extent
>>> of the [[:alnum:]] here, however.
>>
>> Care to explain further? Not to sure what you mean here.
>
> For natural language, it may or may not make sense to match numbers as
> part of a word.
>
> For typical use in e.g. emails, a lot of punctuation has a double role;
> breaking words in
>
>  http://article.gmane.org/gmane.comp.version-control.git/188391
>
> may or may not make sense.
>
> For some uses, especially source code, it would be better to match an
> underscore _ as part of a complete word, too.
>
> For some programming languages, say lisp, a dash - would also belong in
> the same category.
>
> There's no real reason other than ease of implementation why the pattern
> handles ASCII non-alphanumerics separately, but non-ASCII UTF-8
> non-alnums (like, say, unicode NO-BREAK SPACE which would show as \xc2
> \xa0) always goes into a word.  But if you were to make UTF-8 sequences
> a single word, text in (say) many European languages would become
> chunked at accented letters.
>
> I'm sure you can find more items for this list.  It's a grey area.

Thanks.

-- 
Cheers,
Ray Chuan