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

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.

[mpvl, 2012-08-30 16:54:09]

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

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

[r, 2012-08-31 22:30:10]

LGTM

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

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/bu...
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/bu...
File src/pkg/exp/locale/collate/build/colelem.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/bu...
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/bu...
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/bu...
File src/pkg/exp/locale/collate/build/order.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/bu...
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/

[mpvl, 2012-09-01 12:13:58]

*** 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/bu...
File src/pkg/exp/locale/collate/build/builder.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/bu...
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/bu...
File src/pkg/exp/locale/collate/build/colelem.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/bu...
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/bu...
File src/pkg/exp/locale/collate/build/order.go (right):

http://codereview.appspot.com/6493063/diff/1002/src/pkg/exp/locale/collate/bu...
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.