code review 4571074: unicode: add case folding tables

unicode: add case folding tables

[rsc, 2011-06-14 22:23:21]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[rsc, 2011-06-14 22:27:33]

for regexp.

most of the space is for the Ll and Lu exception tables.
i tried a bunch of different things but this was the
simplest i could come up with.

define fold(x) to be the runes equivalent to x.
incredibly, fold(Ll) != Ll+Lu and fold(Lu) != Lu+Ll.
in both cases the fold set is ever so slightly smaller.

russ

[r, 2011-06-14 22:31:33]

quick comments. i'll give another pass a little later

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go
File src/pkg/unicode/letter.go (right):

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:281: // ``simple case folding'' and returns the
resulting slice.
s/fancy quotes/"/g or nothing

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:310: // here can change to uint32.
|cfmt

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:317: // to rune under the ``simple case folding.'' 
For example,
ditto about quotes.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:319: //     'k' -> 'K'  (Kelvin symbol)
add U+ to parens

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:322: lo := 0
code might benefit by coercing rune up top rather than converting everything
that talks to it

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/tables.go
File src/pkg/unicode/tables.go (right):

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/tables.go#new...
src/pkg/unicode/tables.go:5672: var caseOrbit = []foldPair{
blank line wanted before here.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/tables.go#new...
src/pkg/unicode/tables.go:5821: // FoldCategory maps a category name to a table
of
blank line

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/tables.go#new...
src/pkg/unicode/tables.go:5836: var foldLl = &RangeTable{
blank line

[r, 2011-06-15 02:01:02]

code looks good but needs the self-check test

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go
File src/pkg/unicode/letter.go (right):

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:282: func SimpleFold(dst []int, rune int) []int {
can you explain the api? why are dst and a return value both needed? why not
just return a []int of the folding? also, what about duplicates?

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter_test.go
File src/pkg/unicode/letter_test.go (right):

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter_test.g...
src/pkg/unicode/letter_test.go:418: if out := string(SimpleFold(nil, tt.in));
out != tt.out {
i see the dst is nil here, which argues that it can be dropped from the
interface.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/maketables.go
File src/pkg/unicode/maketables.go (right):

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/maketables.go...
src/pkg/unicode/maketables.go:1004: func printCasefold() {
if *test { return }

or - better - test it.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/maketables.go...
src/pkg/unicode/maketables.go:1149: func printCaseOrbit() {
if *test { return }
or - better - test it.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/maketables.go...
src/pkg/unicode/maketables.go:1161: func printCatFold(name string, m
map[string]map[int]bool) {
ditto

[rsc, 2011-06-15 02:16:10]

On Tue, Jun 14, 2011 at 22:01,  <r@golang.org> wrote:
> code looks good but needs the self-check test

will add

>
http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
> src/pkg/unicode/letter.go:282: func SimpleFold(dst []int, rune int)
> []int {
> can you explain the api? why are dst and a return value both needed? why
> not just return a []int of the folding? also, what about duplicates?

it appends to a slice so that a call does not imply an allocation.
in case-insensitive mode this is going to get called on every
character in a regular expression; i want to be able to reuse
the same output buffer.

SimpleFold does not consider the existing values in the slice
when appending new ones.  among the new runes appended,
there are no duplicates.

[r2, 2011-06-15 03:13:40]

On Jun 15, 2011, at 12:15 PM, Russ Cox wrote:

> On Tue, Jun 14, 2011 at 22:01,  <r@golang.org> wrote:
>> code looks good but needs the self-check test
> 
> will add
> 
>>
http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
>> src/pkg/unicode/letter.go:282: func SimpleFold(dst []int, rune int)
>> []int {
>> can you explain the api? why are dst and a return value both needed? why
>> not just return a []int of the folding? also, what about duplicates?
> 
> it appends to a slice so that a call does not imply an allocation.
> in case-insensitive mode this is going to get called on every
> character in a regular expression; i want to be able to reuse
> the same output buffer.

that's what i figured, but it's surprising enough to be mentioned in the
comment.

> SimpleFold does not consider the existing values in the slice
> when appending new ones.  among the new runes appended,
> there are no duplicates.

ditto

[rsc, 2011-06-16 19:20:03]

Hello r@golang.org, r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-16 19:23:07]

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go
File src/pkg/unicode/letter.go (right):

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:281: // ``simple case folding'' and returns the
resulting slice.
On 2011/06/14 22:31:33, r wrote:
> s/fancy quotes/"/g or nothing

The fancy quotes format correctly in godoc (a la troff).

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:282: func SimpleFold(dst []int, rune int) []int {
On 2011/06/15 02:01:02, r wrote:
> can you explain the api? why are dst and a return value both needed? why not
> just return a []int of the folding? also, what about duplicates?

I changed the API.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:310: // here can change to uint32.
On 2011/06/14 22:31:33, r wrote:
> |cfmt

Done.

http://codereview.appspot.com/4571074/diff/2011/src/pkg/unicode/letter.go#new...
src/pkg/unicode/letter.go:322: lo := 0
On 2011/06/14 22:31:33, r wrote:
> code might benefit by coercing rune up top rather than converting everything
> that talks to it

I did it this way so that this code doesn't need to change
if foldPair's field types ever do (from uint16 to uint32).

[r, 2011-06-16 21:23:24]

LGTM

http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/letter.go
File src/pkg/unicode/letter.go (right):

http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/letter.go#ne...
src/pkg/unicode/letter.go:289: // the ``simple case folding.''  Among the code
points equivalent to
i still think that these quotes look terrible and are unnecessary. i'm not even
sure why they're there.

http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/tables.go
File src/pkg/unicode/tables.go (right):

http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/tables.go#ne...
src/pkg/unicode/tables.go:5959: 
now you've got doubled newlines in here. hardly a problem but let's be
consistent.

[rsc, 2011-06-16 21:40:04]

On Thu, Jun 16, 2011 at 17:23,  <r@golang.org> wrote:
> LGTM
>
>
>
> http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/letter.go
> File src/pkg/unicode/letter.go (right):
>
>
http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/letter.go#ne...
> src/pkg/unicode/letter.go:289: // the ``simple case folding.''  Among
> the code points equivalent to
> i still think that these quotes look terrible and are unnecessary. i'm
> not even sure why they're there.

They're there because if I write

// ... according to the Unicode-defined simple case folding

I was worried that it sounded like I was the one calling
the rules simple as opposed to referring the rule whose
name is 'simple case folding'.

I will remove them.

Russ

[r2, 2011-06-16 21:47:30]

On 17/06/2011, at 7:40 AM, Russ Cox wrote:

> On Thu, Jun 16, 2011 at 17:23,  <r@golang.org> wrote:
>> LGTM
>> 
>> 
>> 
>> http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/letter.go
>> File src/pkg/unicode/letter.go (right):
>> 
>>
http://codereview.appspot.com/4571074/diff/15001/src/pkg/unicode/letter.go#ne...
>> src/pkg/unicode/letter.go:289: // the ``simple case folding.''  Among
>> the code points equivalent to
>> i still think that these quotes look terrible and are unnecessary. i'm
>> not even sure why they're there.
> 
> They're there because if I write
> 
> // ... according to the Unicode-defined simple case folding
> 
> I was worried that it sounded like I was the one calling
> the rules simple as opposed to referring the rule whose
> name is 'simple case folding'.
> 
> I will remove them.

the proximity makes the parse clear. you don't need them.

-rob

[rsc, 2011-06-16 21:53:05]

dropped `` '' added Unicode-defined

[rsc, 2011-06-16 21:53:22]

Hello r@golang.org, r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-06-16 21:55:51]

LGTM

[rsc, 2011-06-16 21:56:32]

*** Submitted as http://code.google.com/p/go/source/detail?r=3a81409b9013 ***

unicode: add case folding tables

R=r, r
CC=golang-dev
http://codereview.appspot.com/4571074