code review 6493063: exp/locale/collate: first changes that introduce implem... (issue 6493063)

[mp...@golang.org]

Reviewers: r,

Message:
Hello r...@golang.org (cc: golan...@googlegroups.com),

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


Description:
exp/locale/collate: first changes that introduce implementation of
tailorings:
- Elements in the array are now sorted as a linked list. This makes it
easier to
apply tailorings.
- Added code to sort entries by collation elements.
- Added logical reset points. This is used for tailoring relative to
certain
properties, rather than characters.

NOTE: all code for type entry should now be in order.go. To keep the
diffs for
this CL reasonable, though, the existing code is left in builder.go.
I'll move
this in a separate CL.

Please review this at http://codereview.appspot.com/6493063/

Affected files:
M src/pkg/exp/locale/collate/build/builder.go
M src/pkg/exp/locale/collate/build/builder_test.go
M src/pkg/exp/locale/collate/build/colelem.go
M src/pkg/exp/locale/collate/build/colelem_test.go
A src/pkg/exp/locale/collate/build/order.go
A src/pkg/exp/locale/collate/build/order_test.go

[r...@golang.org]

LGTM


http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/builder.go
File src/pkg/exp/locale/collate/build/builder.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/builder.go#newcode46
src/pkg/exp/locale/collate/build/builder.go:46: noindex bool // do not
include in table
maybe noIndex but i'm not a fan of negative true, so i'd go with
doIndex. but if you need the zero value to be false, maybe "exclude", so
at least we avoid the "no"?

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/colelem.go
File src/pkg/exp/locale/collate/build/colelem.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/colelem.go#newcode236
src/pkg/exp/locale/collate/build/colelem.go:236: for ia < len(a) || ib <
len(b) {
this is fine, but if you want to do it all in the for loop, you could
say

for ia, ib := 0, 0; ia < len(a) || ib < len(b); ia, ib = ia+1, ib+1

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/colelem.go#newcode247
src/pkg/exp/locale/collate/build/colelem.go:247: ib++
you're double-advancing here. i think it's right but just be sure.

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/order.go
File src/pkg/exp/locale/collate/build/order.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/order.go#newcode130
src/pkg/exp/locale/collate/build/order.go:130: // cannot be derived,
i.e. str represents more than one rune.
s/i.e./that is, if/

http://codereview.appspot.com/6493063/

[mp...@golang.org]

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

exp/locale/collate: first changes that introduce implementation of
tailorings:
- Elements in the array are now sorted as a linked list. This makes it
easier to
apply tailorings.
- Added code to sort entries by collation elements.
- Added logical reset points. This is used for tailoring relative to
certain
properties, rather than characters.

NOTE: all code for type entry should now be in order.go. To keep the
diffs for
this CL reasonable, though, the existing code is left in builder.go.
I'll move
this in a separate CL.

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

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/builder.go
File src/pkg/exp/locale/collate/build/builder.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/builder.go#newcode46
src/pkg/exp/locale/collate/build/builder.go:46: noindex bool // do not
include in table

Changed to exclude.

On 2012/08/31 22:30:10, r wrote:
> maybe noIndex but i'm not a fan of negative true, so i'd go with
doIndex. but if
> you need the zero value to be false, maybe "exclude", so at least we
avoid the
> "no"?

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/colelem.go
File src/pkg/exp/locale/collate/build/colelem.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/colelem.go#newcode247
src/pkg/exp/locale/collate/build/colelem.go:247: ib++
Yes, both need to advance. nextVal only advances on zero values.

On 2012/08/31 22:30:10, r wrote:
> you're double-advancing here. i think it's right but just be sure.

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/order.go
File src/pkg/exp/locale/collate/build/order.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/build/order.go#newcode130
src/pkg/exp/locale/collate/build/order.go:130: // cannot be derived,
i.e. str represents more than one rune.

On 2012/08/31 22:30:10, r wrote:
> s/i.e./that is, if/

Done.

http://codereview.appspot.com/6493063/