Experimental alternative implementation of the vector p...

Experimental alternative implementation of the vector package

[jan_mercl, 2009-12-15 10:32:50]

Stage 1 - unmodified container/vector added to exp/vector

[jan_mercl, 2009-12-15 10:41:01]

Oops. I probably should have used "hg copy" instead of "hg add", but noticed the
existence of that option only now. 

Sorry for any inconveniences. Should I redo?

bflm

[rsc, 2009-12-15 17:36:09]

Sorry this has gotten so complicated.  We could probably have
salvaged the original CL, but let's keep using this one.

You don't need to do the two-step process that Robert suggested;
Mercurial is a bit more powerful than the tools we usually use.

I assume you have your actual files saved away somewhere.
If you run:

    hg revert src/pkg/exp/vector
    rm src/pkg/exp/vector/*
        # now you're back to a clean state
    hg cp src/pkg/container/vector/* src/pkg/exp/vector
        # now you're back to having a copy
    hg upload 178048  # (just in case)
        # Before running the next command, copy
        # your new vector implementation into exp/vector.
    hg upload 178048

The diffs that get uploaded by the second upload should be
based on the changes from container/vector, because Mercurial
knows about the cp step.  Even if that fails, we'll be able to
diff against the first upload, which was the raw originals.

Please try that.

Thanks.

[jan_mercl, 2009-12-15 18:24:47]

On 2009/12/15 17:36:09, rsc wrote:
> Sorry this has gotten so complicated.

More of my fault than anyone's else.

> I assume you have your actual files saved away somewhere.

Yes.

> If you run:
...
> Please try that.

Thank you for the detailed instructions, I will hopefuly try to follow them
later this evening/night (it's 7:15PM == CET here)

Thanks for your patience with my chaos creating.

[jan_mercl, 2009-12-15 20:41:14]

I again produced a mess, but this time it was maybe not my fault:

21:28 bflm@tux64:~/go$ hg upload 178048
abort: HTTP Error 500: Internal Server Error
21:31 bflm@tux64:~/go$ hg upload 178048
Loading /178048/edit: urllib2.HTTPError: HTTP Error 500: Internal Server Error;
trying again in 2 seconds.
Loading /178048/edit: urllib2.HTTPError: HTTP Error 500: Internal Server Error;
trying again in 2 seconds.
http://codereview.appspot.com/178048

21:32 bflm@tux64:~/go$

But not doing hg add for the other new files _is_ my fault. Can't believe myself
what am I doing today. Apologizes to all.

[jan_mercl, 2009-12-15 20:54:03]

hg added exp_vector_test.go numbers_test.go nums.sh
hg uploaded => patch set 5, but those files still not there

I give up, please salvage those 3 files from the initial CL 176053

Thanks in advance for not shotting me on the spot, as I would like to do it by
myself (not seriously)

bflm

[rsc, 2009-12-15 21:08:34]

i'll help you over chat.
the 500 errors are not your fault.

[rsc, 2009-12-15 22:05:16]

ready for gri

[gri, 2009-12-16 03:29:53]

Looks pretty good.

Some initial comments.

I guess some of the performance is stemming from the fact that intVectors are
not wrappers anymore which saved a level of indirection.

However, not intVector and StringVector are essentially copies of vector with
different element type - this is one of the more clear-cut cases where one would
want to use genericity.

I am not too happy with the code triplication. How about trying to generate the
versions? If there was a (non-compilable) template using an element type T, you
might be able to use gofmt -r to produce three versions.

http://codereview.appspot.com/178048/diff/1062/2037
File src/pkg/exp/vector/numbers_test.go (right):

http://codereview.appspot.com/178048/diff/1062/2037#newcode1
src/pkg/exp/vector/numbers_test.go:1: package vector
needs copyright notice

http://codereview.appspot.com/178048/diff/1062/2037#newcode3
src/pkg/exp/vector/numbers_test.go:3: import "fmt"
use import() grouping

http://codereview.appspot.com/178048/diff/1062/2038
File src/pkg/exp/vector/nums.sh (right):

http://codereview.appspot.com/178048/diff/1062/2038#newcode1
src/pkg/exp/vector/nums.sh:1: gotest -v -match Nums -benchmarks Nums
should probably have the copyright notice

http://codereview.appspot.com/178048/diff/1062/2040
File src/pkg/exp/vector/vector.go (left):

http://codereview.appspot.com/178048/diff/1062/2040#oldcode32
src/pkg/exp/vector/vector.go:32: a := p.a;
Please keep the temporary a. It's easier to read and no extra deref is necessary
at each use.

a := *p

http://codereview.appspot.com/178048/diff/1062/2040#oldcode56
src/pkg/exp/vector/vector.go:56: p.a = a;
*p = a

http://codereview.appspot.com/178048/diff/1062/2040#oldcode67
src/pkg/exp/vector/vector.go:67: a := p.a;
a := *p

http://codereview.appspot.com/178048/diff/1062/2040#oldcode79
src/pkg/exp/vector/vector.go:79: p.a = a[0:length];
*p = a[0:length];

http://codereview.appspot.com/178048/diff/1062/2040
File src/pkg/exp/vector/vector.go (right):

http://codereview.appspot.com/178048/diff/1062/2040#newcode10
src/pkg/exp/vector/vector.go:10: // initial underlying array size
s/initial/Initial/

http://codereview.appspot.com/178048/diff/1062/2040#newcode14
src/pkg/exp/vector/vector.go:14: // Vector is a generic container for
interface{} items = any type.
Vector is a container for numbered sequences of elements of type interface{}. A
vector's length and capacity adjusts automatically as necessary. The zero
value...

http://codereview.appspot.com/178048/diff/1062/2040#newcode28
src/pkg/exp/vector/vector.go:28: func (p *Vector) expand(i, n int) {
This function is now useful outside this package. Export it.

s/expand/Expand/

It probably makes sense to also have

Extend(n int) == Expand(len(v), n)

http://codereview.appspot.com/178048/diff/1062/2040#newcode78
src/pkg/exp/vector/vector.go:78: // Len returns the number of elements in the
vector.
Add:
// Same as len(*p).

http://codereview.appspot.com/178048/diff/1062/2040#newcode83
src/pkg/exp/vector/vector.go:83: // maximum length the vector can grow without
resizing.
Add:
// Same as cap(*p).

http://codereview.appspot.com/178048/diff/1062/2040#newcode118
src/pkg/exp/vector/vector.go:118: n := len(*p);
probably better to use the a alias in here

http://codereview.appspot.com/178048/diff/1062/2040#newcode129
src/pkg/exp/vector/vector.go:129: p.expand(i, len(*x));
b := *x (?)

http://codereview.appspot.com/178048/diff/1062/2040#newcode136
src/pkg/exp/vector/vector.go:136: n := len(*p);
keep using the a alias

http://codereview.appspot.com/178048/diff/1062/2040#newcode151
src/pkg/exp/vector/vector.go:151: s := make([]interface{}, j-i);	// will fail in
Init() if j < i
Here the minimum capacity is not bootstrap.

Perhaps have a helper function that does allocation everywhere and get rid of
the max function?

http://codereview.appspot.com/178048/diff/1062/2040#newcode160
src/pkg/exp/vector/vector.go:160: for i := 0; i < len(*p); i++ {
a := *p, then use a

http://codereview.appspot.com/178048/diff/1062/2040#newcode174
src/pkg/exp/vector/vector.go:174: i := len(*p) - 1;
a := *p

http://codereview.appspot.com/178048/diff/1062/2040#newcode199
src/pkg/exp/vector/vector.go:199: func (p *Vector) Swap(i, j int)	{ (*p)[i],
(*p)[j] = (*p)[j], (*p)[i] }
a := *p

http://codereview.appspot.com/178048/diff/1062/2041
File src/pkg/exp/vector/vector_test.go (right):

http://codereview.appspot.com/178048/diff/1062/2041#newcode14
src/pkg/exp/vector/vector_test.go:14: func TestZeroLenExp(t *testing.T) {
I assume the rename is needed because of conflicts when importing both
container/vector and exp/vector? (which is a known compiler bug)

http://codereview.appspot.com/178048/diff/1062/2041#newcode17
src/pkg/exp/vector/vector_test.go:17: t.Errorf("B) expected 0, got %d", a.Len())
s/"B"/"B1"/ for consistency ?

http://codereview.appspot.com/178048/diff/1062/2041#newcode35
src/pkg/exp/vector/vector_test.go:35: t.Errorf("Bi) expected 0, got %d",
a.Len())
s/Bi/B1i

http://codereview.appspot.com/178048/diff/1062/2041#newcode53
src/pkg/exp/vector/vector_test.go:53: t.Errorf("Bs) expected 0, got %d",
a.Len())
s/Bs/B1s/

[jan_mercl, 2009-12-16 21:03:08]

On 2009/12/16 03:29:53, gri wrote:
> I guess some of the performance is stemming from the fact that intVectors are
> not wrappers anymore which saved a level of indirection.

That't true I think.

> I am not too happy with the code triplication. How about trying to generate
the
> versions? If there was a (non-compilable) template using an element type T,
you
> might be able to use gofmt -r to produce three versions.

The template is in vector.got and make generates the Go sources.

bflm

[jan_mercl, 2009-12-16 21:04:29]

Again stuck with mercurial:

$ hg upload 178048
Got error status from ['hg', 'cat', '-r', 'b0524afd0d7c',
'src/pkg/exp/vector/intvector.go']:
$ $ hg pe
178048:
	Experimental alternative implementation of the vector package

	Reviewer: gri
	CC: rsc
	Files:
		src/pkg/exp/vector/Makefile
		src/pkg/exp/vector/intvector.go
		src/pkg/exp/vector/numbers_test.go
		src/pkg/exp/vector/nums.sh
		src/pkg/exp/vector/stringvector.go
		src/pkg/exp/vector/vector.go
		src/pkg/exp/vector/vector.got
		src/pkg/exp/vector/vector_test.go

$ hg st
M src/pkg/exp/vector/intvector.go
M src/pkg/exp/vector/stringvector.go
M src/pkg/exp/vector/vector.go
M src/pkg/exp/vector/vector_test.go
A src/pkg/exp/vector/Makefile
A src/pkg/exp/vector/numbers_test.go
A src/pkg/exp/vector/nums.sh
A src/pkg/exp/vector/vector.got
$ 

Please hint me how to get the changes to the server. I'm out of ideas how to do
it.

bflm

http://codereview.appspot.com/178048/diff/1062/2037
File src/pkg/exp/vector/numbers_test.go (right):

http://codereview.appspot.com/178048/diff/1062/2037#newcode1
src/pkg/exp/vector/numbers_test.go:1: package vector
On 2009/12/16 03:29:53, gri wrote:
> needs copyright notice

Done.

http://codereview.appspot.com/178048/diff/1062/2037#newcode3
src/pkg/exp/vector/numbers_test.go:3: import "fmt"
On 2009/12/16 03:29:53, gri wrote:
> use import() grouping

Done.

http://codereview.appspot.com/178048/diff/1062/2038
File src/pkg/exp/vector/nums.sh (right):

http://codereview.appspot.com/178048/diff/1062/2038#newcode1
src/pkg/exp/vector/nums.sh:1: gotest -v -match Nums -benchmarks Nums
On 2009/12/16 03:29:53, gri wrote:
> should probably have the copyright notice

Done.

http://codereview.appspot.com/178048/diff/1062/2040
File src/pkg/exp/vector/vector.go (left):

http://codereview.appspot.com/178048/diff/1062/2040#oldcode32
src/pkg/exp/vector/vector.go:32: a := p.a;
On 2009/12/16 03:29:53, gri wrote:
> Please keep the temporary a. It's easier to read and no extra deref is
necessary
> at each use.
> 
> a := *p

Done.

http://codereview.appspot.com/178048/diff/1062/2040#oldcode56
src/pkg/exp/vector/vector.go:56: p.a = a;
On 2009/12/16 03:29:53, gri wrote:
> *p = a

Done.

http://codereview.appspot.com/178048/diff/1062/2040#oldcode67
src/pkg/exp/vector/vector.go:67: a := p.a;
On 2009/12/16 03:29:53, gri wrote:
> a := *p

Done.

http://codereview.appspot.com/178048/diff/1062/2040#oldcode79
src/pkg/exp/vector/vector.go:79: p.a = a[0:length];
On 2009/12/16 03:29:53, gri wrote:
> *p = a[0:length];

Done.

http://codereview.appspot.com/178048/diff/1062/2040
File src/pkg/exp/vector/vector.go (right):

http://codereview.appspot.com/178048/diff/1062/2040#newcode10
src/pkg/exp/vector/vector.go:10: // initial underlying array size
On 2009/12/16 03:29:53, gri wrote:
> s/initial/Initial/

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode14
src/pkg/exp/vector/vector.go:14: // Vector is a generic container for
interface{} items = any type.
On 2009/12/16 03:29:53, gri wrote:
> Vector is a container for numbered sequences of elements of type interface{}.
A
> vector's length and capacity adjusts automatically as necessary. The zero
> value...

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode28
src/pkg/exp/vector/vector.go:28: func (p *Vector) expand(i, n int) {
On 2009/12/16 03:29:53, gri wrote:
> This function is now useful outside this package. Export it.
> 
> s/expand/Expand/
> 
> It probably makes sense to also have
> 
> Extend(n int) == Expand(len(v), n)
> 
> 

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode78
src/pkg/exp/vector/vector.go:78: // Len returns the number of elements in the
vector.
On 2009/12/16 03:29:53, gri wrote:
> Add:
> // Same as len(*p).

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode83
src/pkg/exp/vector/vector.go:83: // maximum length the vector can grow without
resizing.
On 2009/12/16 03:29:53, gri wrote:
> Add:
> // Same as cap(*p).

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode118
src/pkg/exp/vector/vector.go:118: n := len(*p);
On 2009/12/16 03:29:53, gri wrote:
> probably better to use the a alias in here

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode129
src/pkg/exp/vector/vector.go:129: p.expand(i, len(*x));
On 2009/12/16 03:29:53, gri wrote:
> b := *x (?)

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode136
src/pkg/exp/vector/vector.go:136: n := len(*p);
On 2009/12/16 03:29:53, gri wrote:
> keep using the a alias

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode151
src/pkg/exp/vector/vector.go:151: s := make([]interface{}, j-i);	// will fail in
Init() if j < i
On 2009/12/16 03:29:53, gri wrote:
> Here the minimum capacity is not bootstrap.
> 
> Perhaps have a helper function that does allocation everywhere and get rid of
> the max function?

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode160
src/pkg/exp/vector/vector.go:160: for i := 0; i < len(*p); i++ {
On 2009/12/16 03:29:53, gri wrote:
> a := *p, then use a

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode174
src/pkg/exp/vector/vector.go:174: i := len(*p) - 1;
On 2009/12/16 03:29:53, gri wrote:
> a := *p

Done.

http://codereview.appspot.com/178048/diff/1062/2040#newcode199
src/pkg/exp/vector/vector.go:199: func (p *Vector) Swap(i, j int)	{ (*p)[i],
(*p)[j] = (*p)[j], (*p)[i] }
On 2009/12/16 03:29:53, gri wrote:
> a := *p

Done.

http://codereview.appspot.com/178048/diff/1062/2041
File src/pkg/exp/vector/vector_test.go (right):

http://codereview.appspot.com/178048/diff/1062/2041#newcode14
src/pkg/exp/vector/vector_test.go:14: func TestZeroLenExp(t *testing.T) {
On 2009/12/16 03:29:53, gri wrote:
> I assume the rename is needed because of conflicts when importing both
> container/vector and exp/vector? (which is a known compiler bug)

The rename was caused by using as a reference the original vector_test at the
same time as exp_vector_test and gotest rightfully complained. Could be now
reverted, but that'll hit the bug you mentioned in a possible testing scenario.

http://codereview.appspot.com/178048/diff/1062/2041#newcode17
src/pkg/exp/vector/vector_test.go:17: t.Errorf("B) expected 0, got %d", a.Len())
On 2009/12/16 03:29:53, gri wrote:
> s/"B"/"B1"/ for consistency ?

Done.

http://codereview.appspot.com/178048/diff/1062/2041#newcode35
src/pkg/exp/vector/vector_test.go:35: t.Errorf("Bi) expected 0, got %d",
a.Len())
On 2009/12/16 03:29:53, gri wrote:
> s/Bi/B1i

Done.

http://codereview.appspot.com/178048/diff/1062/2041#newcode53
src/pkg/exp/vector/vector_test.go:53: t.Errorf("Bs) expected 0, got %d",
a.Len())
On 2009/12/16 03:29:53, gri wrote:
> s/Bs/B1s/

Done.

[rsc, 2009-12-16 21:34:13]

On Wed, Dec 16, 2009 at 13:04,  <befelemepeseveze@gmail.com> wrote:
> Again stuck with mercurial:
>
> $ hg upload 178048
> Got error status from ['hg', 'cat', '-r', 'b0524afd0d7c',
> 'src/pkg/exp/vector/intvector.go']:

Can you try running the hg upload again and see if it happens again?

If so, can you try running the command it says is failing and
see what happens (hg cat -r b0524afd0d7c src/pkg/exp/vector/intvector.go)?
I've never seen this before.

> A src/pkg/exp/vector/vector.got

Looks like you may also need to assign
this file to the CL:

hg file 178048 src/pkg/exp/vector/vector.got

but that wouldn't explain the upload problem.

Russ

[jan_mercl, 2009-12-16 21:41:14]

On 2009/12/16 21:34:13, rsc wrote:
> On Wed, Dec 16, 2009 at 13:04,  <mailto:befelemepeseveze@gmail.com> wrote:

Reverted synced re-added re-filed and finally uploaded.

bflm

[gri, 2009-12-17 00:42:20]

This looks much better already, but I think we should be able to do this without
m4 (and then won't need a extra step through gofmt after using m4).

I looked at the diffs between vector.go and intvector.go, and intvector.go and
stringvector.go. Observations:

- Comments: Almost all comments can remain the same with some exceptions (see
file/line-specific feedback).
- Some customizations can be done generically or factored out.

Thus:

- Add an extra file that contains the package comment, the actual type
declarations with corresponding comments, and whatever specific functionality
cannot be written generically without the m4_ifelse.

The extension .got conflicts with .go when using terminal filename completion.
It would be nicer if one could simply customize vector.go without an extra copy.


A minor fix to gofmt -r is under way which will allow you to do just that:

gofmt -r 'interface{} -> int' vector.go > intvector.go

should work shortly. Thus, you can get rid of vector.got, and with a little bit
of factoring into a separate file (say, defs.go) intvector.go and
stringvector.go can be directly generated from vector.go.

- gri

http://codereview.appspot.com/178048/diff/1077/2053
File src/pkg/exp/vector/intvector.go (right):

http://codereview.appspot.com/178048/diff/1077/2053#newcode21
src/pkg/exp/vector/intvector.go:21: // IntVector is a container for numbered
sequences of elements of type int.
These are the only comments that need type-specific replacement. An easy way out
is to move the three declarations for Vector, IntVector, StringVector into a
separate file which also contains the package content.

http://codereview.appspot.com/178048/diff/1077/2053#newcode63
src/pkg/exp/vector/intvector.go:63: // Resize adds 0 elements. The capacity
parameter is ignored unless the
change into:

..., Resize adds the respective zero values for the additional elements. ...

So this comment can be used generically

http://codereview.appspot.com/178048/diff/1077/2053#newcode155
src/pkg/exp/vector/intvector.go:155: // Slice returns a new IntVector by slicing
the old one to extract slice [i:j].
s/IntVector/sub-vector/

so it can be used generically

http://codereview.appspot.com/178048/diff/1077/2053#newcode193
src/pkg/exp/vector/intvector.go:193: // AppendVector appends the entire
IntVector x to the end of this vector.
s/IntVector/vector/

http://codereview.appspot.com/178048/diff/1077/2058
File src/pkg/exp/vector/vector.got (right):

http://codereview.appspot.com/178048/diff/1077/2058#newcode11
src/pkg/exp/vector/vector.got:11: m4_ifelse(T,`Vector',`// The vector package
implements a container for managing sequences
Move the package comment out into a separate file which is written manually.
Then you don't need this.

http://codereview.appspot.com/178048/diff/1077/2058#newcode16
src/pkg/exp/vector/vector.got:16: m4_ifelse(T,`Vector',`// Initial underlying
array size
Same with this constant.

http://codereview.appspot.com/178048/diff/1077/2058#newcode85
src/pkg/exp/vector/vector.got:85: a[length+i] = NULL
var zero T
a[length+i] = zero

http://codereview.appspot.com/178048/diff/1077/2058#newcode140
src/pkg/exp/vector/vector.got:140: m4_ifelse(T,`Vector',`	a[n-1] = nil //
support GC, nil out entry
Ok to do this always for all vectors:

var zero T
a[n-1] = zero

http://codereview.appspot.com/178048/diff/1077/2058#newcode162
src/pkg/exp/vector/vector.got:162: m4_ifelse(T,`Vector',`	for k := m; k < n; k++
{
This is the one place where we probably don't want to zero out the elements
unless it's a Vector. One could use a Go if statement with a constant that is
appropriately defined (if the constant is false, no code will be generated for
the if). Or, to get going, for now one could do it always and put in a TODO
(that's what I would do for now).

http://codereview.appspot.com/178048/diff/1077/2058#newcode223
src/pkg/exp/vector/vector.got:223: // Less returns a boolean denoting whether
the i'th element is less than the j'th element.
This function is so small, just have it in the manually written file for all
three types. No need for the m4_ifelse.

[gri, 2009-12-17 00:48:20]

One item I forgot:

You should be able to do the same with the test cases. Instead of having one
huge test file, have multiple test files for the different types, where all
but the first one are generated automatically.

- Robert

On Wed, Dec 16, 2009 at 4:42 PM, <gri@golang.org> wrote:

> This looks much better already, but I think we should be able to do this
> without m4 (and then won't need a extra step through gofmt after using
> m4).
>
> I looked at the diffs between vector.go and intvector.go, and
> intvector.go and stringvector.go. Observations:
>
> - Comments: Almost all comments can remain the same with some exceptions
> (see file/line-specific feedback).
> - Some customizations can be done generically or factored out.
>
> Thus:
>
> - Add an extra file that contains the package comment, the actual type
> declarations with corresponding comments, and whatever specific
> functionality cannot be written generically without the m4_ifelse.
>
> The extension .got conflicts with .go when using terminal filename
> completion. It would be nicer if one could simply customize vector.go
> without an extra copy.
>
> A minor fix to gofmt -r is under way which will allow you to do just
> that:
>
> gofmt -r 'interface{} -> int' vector.go > intvector.go
>
> should work shortly. Thus, you can get rid of vector.got, and with a
> little bit of factoring into a separate file (say, defs.go) intvector.go
> and stringvector.go can be directly generated from vector.go.
>
> - gri
>
>
>
> http://codereview.appspot.com/178048/diff/1077/2053
> File src/pkg/exp/vector/intvector.go (right):
>
> http://codereview.appspot.com/178048/diff/1077/2053#newcode21
> src/pkg/exp/vector/intvector.go:21: // IntVector is a container for
> numbered sequences of elements of type int.
> These are the only comments that need type-specific replacement. An easy
> way out is to move the three declarations for Vector, IntVector,
> StringVector into a separate file which also contains the package
> content.
>
> http://codereview.appspot.com/178048/diff/1077/2053#newcode63
> src/pkg/exp/vector/intvector.go:63: // Resize adds 0 elements. The
> capacity parameter is ignored unless the
> change into:
>
> ..., Resize adds the respective zero values for the additional elements.
> ...
>
> So this comment can be used generically
>
> http://codereview.appspot.com/178048/diff/1077/2053#newcode155
> src/pkg/exp/vector/intvector.go:155: // Slice returns a new IntVector by
> slicing the old one to extract slice [i:j].
> s/IntVector/sub-vector/
>
> so it can be used generically
>
> http://codereview.appspot.com/178048/diff/1077/2053#newcode193
> src/pkg/exp/vector/intvector.go:193: // AppendVector appends the entire
> IntVector x to the end of this vector.
> s/IntVector/vector/
>
> http://codereview.appspot.com/178048/diff/1077/2058
> File src/pkg/exp/vector/vector.got (right):
>
> http://codereview.appspot.com/178048/diff/1077/2058#newcode11
> src/pkg/exp/vector/vector.got:11: m4_ifelse(T,`Vector',`// The vector
> package implements a container for managing sequences
> Move the package comment out into a separate file which is written
> manually. Then you don't need this.
>
> http://codereview.appspot.com/178048/diff/1077/2058#newcode16
> src/pkg/exp/vector/vector.got:16: m4_ifelse(T,`Vector',`// Initial
> underlying array size
> Same with this constant.
>
> http://codereview.appspot.com/178048/diff/1077/2058#newcode85
> src/pkg/exp/vector/vector.got:85: a[length+i] = NULL
> var zero T
> a[length+i] = zero
>
> http://codereview.appspot.com/178048/diff/1077/2058#newcode140
> src/pkg/exp/vector/vector.got:140: m4_ifelse(T,`Vector',`       a[n-1] =
> nil
> // support GC, nil out entry
> Ok to do this always for all vectors:
>
> var zero T
> a[n-1] = zero
>
> http://codereview.appspot.com/178048/diff/1077/2058#newcode162
> src/pkg/exp/vector/vector.got:162: m4_ifelse(T,`Vector',`       for k := m;
> k
> < n; k++ {
> This is the one place where we probably don't want to zero out the
> elements unless it's a Vector. One could use a Go if statement with a
> constant that is appropriately defined (if the constant is false, no
> code will be generated for the if). Or, to get going, for now one could
> do it always and put in a TODO (that's what I would do for now).
>
> http://codereview.appspot.com/178048/diff/1077/2058#newcode223
> src/pkg/exp/vector/vector.got:223: // Less returns a boolean denoting
> whether the i'th element is less than the j'th element.
> This function is so small, just have it in the manually written file for
> all three types. No need for the m4_ifelse.
>
>
> http://codereview.appspot.com/178048
>

[jan_mercl, 2009-12-17 14:16:16]

http://codereview.appspot.com/178048/diff/1077/2053
File src/pkg/exp/vector/intvector.go (right):

http://codereview.appspot.com/178048/diff/1077/2053#newcode21
src/pkg/exp/vector/intvector.go:21: // IntVector is a container for numbered
sequences of elements of type int.
On 2009/12/17 00:42:20, gri wrote:
> These are the only comments that need type-specific replacement. An easy way
out
> is to move the three declarations for Vector, IntVector, StringVector into a
> separate file which also contains the package content.

Done.

http://codereview.appspot.com/178048/diff/1077/2053#newcode63
src/pkg/exp/vector/intvector.go:63: // Resize adds 0 elements. The capacity
parameter is ignored unless the
On 2009/12/17 00:42:20, gri wrote:
> change into:
> 
> ..., Resize adds the respective zero values for the additional elements. ...
> 
> So this comment can be used generically

Done.

http://codereview.appspot.com/178048/diff/1077/2053#newcode155
src/pkg/exp/vector/intvector.go:155: // Slice returns a new IntVector by slicing
the old one to extract slice [i:j].
On 2009/12/17 00:42:20, gri wrote:
> s/IntVector/sub-vector/
> 
> so it can be used generically

Done.

http://codereview.appspot.com/178048/diff/1077/2053#newcode193
src/pkg/exp/vector/intvector.go:193: // AppendVector appends the entire
IntVector x to the end of this vector.
On 2009/12/17 00:42:20, gri wrote:
> s/IntVector/vector/

Done.

http://codereview.appspot.com/178048/diff/1077/2058
File src/pkg/exp/vector/vector.got (right):

http://codereview.appspot.com/178048/diff/1077/2058#newcode11
src/pkg/exp/vector/vector.got:11: m4_ifelse(T,`Vector',`// The vector package
implements a container for managing sequences
On 2009/12/17 00:42:20, gri wrote:
> Move the package comment out into a separate file which is written manually.
> Then you don't need this.

Done.

http://codereview.appspot.com/178048/diff/1077/2058#newcode16
src/pkg/exp/vector/vector.got:16: m4_ifelse(T,`Vector',`// Initial underlying
array size
On 2009/12/17 00:42:20, gri wrote:
> Same with this constant.

Done.

http://codereview.appspot.com/178048/diff/1077/2058#newcode85
src/pkg/exp/vector/vector.got:85: a[length+i] = NULL
On 2009/12/17 00:42:20, gri wrote:
> var zero T
> a[length+i] = zero

Done.

http://codereview.appspot.com/178048/diff/1077/2058#newcode140
src/pkg/exp/vector/vector.got:140: m4_ifelse(T,`Vector',`	a[n-1] = nil //
support GC, nil out entry
On 2009/12/17 00:42:20, gri wrote:
> Ok to do this always for all vectors:
> 
> var zero T
> a[n-1] = zero

Done.

http://codereview.appspot.com/178048/diff/1077/2058#newcode162
src/pkg/exp/vector/vector.got:162: m4_ifelse(T,`Vector',`	for k := m; k < n; k++
{
On 2009/12/17 00:42:20, gri wrote:
> This is the one place where we probably don't want to zero out the elements
> unless it's a Vector. One could use a Go if statement with a constant that is
> appropriately defined (if the constant is false, no code will be generated for
> the if). Or, to get going, for now one could do it always and put in a TODO
> (that's what I would do for now).

Done.

http://codereview.appspot.com/178048/diff/1077/2058#newcode223
src/pkg/exp/vector/vector.got:223: // Less returns a boolean denoting whether
the i'th element is less than the j'th element.
On 2009/12/17 00:42:20, gri wrote:
> This function is so small, just have it in the manually written file for all
> three types. No need for the m4_ifelse.

Done.

[jan_mercl, 2009-12-17 14:16:36]

On 2009/12/17 00:48:20, gri wrote:
> One item I forgot:
> 
> You should be able to do the same with the test cases. Instead of having one
> huge test file, have multiple test files for the different types, where all
> but the first one are generated automatically.
> 
> - Robert

Tried but had to postpone this:

15:00 myname@tux64:~/go/src/pkg/exp/vector$ hg id ; gotest thisfails_test.go
28cb094e4ff1+ tip
throw: interface conversion

panic PC=0x7f1dfcfa80c0
throw+0x3e /home/myname/go/src/pkg/runtime/runtime.c:74
	throw(0x491f38, 0x0)
runtime·ifaceI2T+0x54 /home/myname/go/src/pkg/runtime/iface.c:217
	runtime·ifaceI2T(0x4b7f40, 0x0, 0x0, 0x0)
main·match+0x46a /home/myname/go/src/cmd/gofmt/rewrite.go:152
	main·match(0xfcf77a10, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf73320, ...)
main·_func_003+0x119 /home/myname/go/src/cmd/gofmt/rewrite.go:60
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0x1d5 /home/myname/go/src/cmd/gofmt/rewrite.go:94
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf738c0, ...)
main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0xf7 /home/myname/go/src/cmd/gofmt/rewrite.go:98
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf755a0, 0x7f1d, 0xfcf738a0, ...)
main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0x29e /home/myname/go/src/cmd/gofmt/rewrite.go:89
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf75540, 0x7f1d, 0xfcf73880, ...)
main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0x1d5 /home/myname/go/src/cmd/gofmt/rewrite.go:94
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf736c0, ...)
main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0xf7 /home/myname/go/src/cmd/gofmt/rewrite.go:98
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf755a0, 0x7f1d, 0xfcf736a0, ...)
main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0x29e /home/myname/go/src/cmd/gofmt/rewrite.go:89
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf75540, 0x7f1d, 0xfcf73680, ...)
main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
	main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0, ...)
main·apply+0x1d5 /home/myname/go/src/cmd/gofmt/rewrite.go:94
	main·apply(0xfcf781e0, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf732e0, ...)
main·rewriteFile+0x196 /home/myname/go/src/cmd/gofmt/rewrite.go:65
	main·rewriteFile(0xfcf71510, 0x7f1d, 0xfcf714e0, 0x7f1d, 0xfcf71510, ...)
main·_func_002+0x38 /home/myname/go/src/cmd/gofmt/rewrite.go:31
	main·_func_002(0xfcf74780, 0x7f1d, 0xfcf74790, 0x7f1d, 0x400ba6, ...)
main·processFile+0x166 /home/myname/go/src/cmd/gofmt/gofmt.go:113
	main·processFile(0xfcf71570, 0x7f1d, 0x11, 0x7f1d)
main·processFileByName+0x8c /home/myname/go/src/cmd/gofmt/gofmt.go:149
	main·processFileByName(0xfcf73020, 0x7f1d, 0x11, 0x7f1d, 0x0, ...)
main·main+0x1e1 /home/myname/go/src/cmd/gofmt/gofmt.go:213
	main·main()
mainstart+0xf /home/myname/go/src/pkg/runtime/amd64/asm.s:54
	mainstart()
goexit /home/myname/go/src/pkg/runtime/proc.c:136
	goexit()
/dev/stdin:1:1: expected 'package', found 'interface'
15:07 myname@tux64:~/go/src/pkg/exp/vector$

I need to know your opinion - it's a bug in gofmt rewrite or I'm doing something
conceptually wrong?

Thanks in advance for any help.

bflm

[gri, 2009-12-17 19:44:03]

This looks like a gofmt rewrite bug. Looking into it.
- Robert

On Thu, Dec 17, 2009 at 6:16 AM, <befelemepeseveze@gmail.com> wrote:

> On 2009/12/17 00:48:20, gri wrote:
>
>> One item I forgot:
>>
>
>  You should be able to do the same with the test cases. Instead of
>>
> having one
>
>> huge test file, have multiple test files for the different types,
>>
> where all
>
>> but the first one are generated automatically.
>>
>
>  - Robert
>>
>
> Tried but had to postpone this:
>
> 15:00 myname@tux64:~/go/src/pkg/exp/vector$ hg id ; gotest
> thisfails_test.go
> 28cb094e4ff1+ tip
> throw: interface conversion
>
> panic PC=0x7f1dfcfa80c0
> throw+0x3e /home/myname/go/src/pkg/runtime/runtime.c:74
>        throw(0x491f38, 0x0)
> runtime·ifaceI2T+0x54 /home/myname/go/src/pkg/runtime/iface.c:217
>        runtime·ifaceI2T(0x4b7f40, 0x0, 0x0, 0x0)
> main·match+0x46a /home/myname/go/src/cmd/gofmt/rewrite.go:152
>        main·match(0xfcf77a10, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf73320, ...)
> main·_func_003+0x119 /home/myname/go/src/cmd/gofmt/rewrite.go:60
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0x1d5 /home/myname/go/src/cmd/gofmt/rewrite.go:94
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf738c0, ...)
> main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0xf7 /home/myname/go/src/cmd/gofmt/rewrite.go:98
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf755a0, 0x7f1d, 0xfcf738a0, ...)
> main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0x29e /home/myname/go/src/cmd/gofmt/rewrite.go:89
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf75540, 0x7f1d, 0xfcf73880, ...)
> main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0x1d5 /home/myname/go/src/cmd/gofmt/rewrite.go:94
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf736c0, ...)
> main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0xf7 /home/myname/go/src/cmd/gofmt/rewrite.go:98
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf755a0, 0x7f1d, 0xfcf736a0, ...)
> main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0x29e /home/myname/go/src/cmd/gofmt/rewrite.go:89
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf75540, 0x7f1d, 0xfcf73680, ...)
> main·_func_003+0xd1 /home/myname/go/src/cmd/gofmt/rewrite.go:59
>        main·_func_003(0xfcf702a0, 0x7f1d, 0xfcf702b8, 0x7f1d, 0xfcf748c0,
> ...)
> main·apply+0x1d5 /home/myname/go/src/cmd/gofmt/rewrite.go:94
>        main·apply(0xfcf781e0, 0x7f1d, 0xfcf754e0, 0x7f1d, 0xfcf732e0, ...)
> main·rewriteFile+0x196 /home/myname/go/src/cmd/gofmt/rewrite.go:65
>        main·rewriteFile(0xfcf71510, 0x7f1d, 0xfcf714e0, 0x7f1d, 0xfcf71510,
> ...)
> main·_func_002+0x38 /home/myname/go/src/cmd/gofmt/rewrite.go:31
>        main·_func_002(0xfcf74780, 0x7f1d, 0xfcf74790, 0x7f1d, 0x400ba6,
> ...)
> main·processFile+0x166 /home/myname/go/src/cmd/gofmt/gofmt.go:113
>        main·processFile(0xfcf71570, 0x7f1d, 0x11, 0x7f1d)
> main·processFileByName+0x8c /home/myname/go/src/cmd/gofmt/gofmt.go:149
>        main·processFileByName(0xfcf73020, 0x7f1d, 0x11, 0x7f1d, 0x0, ...)
> main·main+0x1e1 /home/myname/go/src/cmd/gofmt/gofmt.go:213
>        main·main()
> mainstart+0xf /home/myname/go/src/pkg/runtime/amd64/asm.s:54
>        mainstart()
> goexit /home/myname/go/src/pkg/runtime/proc.c:136
>        goexit()
> /dev/stdin:1:1: expected 'package', found 'interface'
> 15:07 myname@tux64:~/go/src/pkg/exp/vector$
>
> I need to know your opinion - it's a bug in gofmt rewrite or I'm doing
> something conceptually wrong?
>
> Thanks in advance for any help.
>
> bflm
>
>
> http://codereview.appspot.com/178048
>

[gri, 2009-12-17 23:14:19]

Looks great. I think once we have the test files generated as well this is good
to go.

Fixed version of gofmt under review.

http://codereview.appspot.com/178048/diff/2069/2070
File src/pkg/exp/vector/Makefile (right):

http://codereview.appspot.com/178048/diff/2069/2070#newcode14
src/pkg/exp/vector/Makefile:14: intvector.go: vector.go
Note that this may cause this rule to be invoked (say after a fresh installation
if the time stamps are off by a ms) before gofmt exist and thus won't work.
Probably need a separate generate rule which should be ok since the generated
files are checked in anyway (for the above reason).

http://codereview.appspot.com/178048/diff/2069/2071
File src/pkg/exp/vector/defs.go (right):

http://codereview.appspot.com/178048/diff/2069/2071#newcode5
src/pkg/exp/vector/defs.go:5: // The vector package implements a container for
managing sequences
s/a container/containers/

(there's three kinds)

http://codereview.appspot.com/178048/diff/2069/2071#newcode55
src/pkg/exp/vector/defs.go:55: // The function should not change the indexing of
the vector underfoot.
Instead of the 2nd line write:

The behavior of Do is undefined if f changes *p.

http://codereview.appspot.com/178048/diff/2069/2071#newcode59
src/pkg/exp/vector/defs.go:59: for i := 0; i < len(a); i++ {
actually, you should be able to use range here now:

for _, e := range *p {
   f(e)
}

http://codereview.appspot.com/178048/diff/2069/2071#newcode60
src/pkg/exp/vector/defs.go:60: f(a[i]) // not too safe if f changes the Vector
instead of having this comment, document the interface

http://codereview.appspot.com/178048/diff/2069/2071#newcode66
src/pkg/exp/vector/defs.go:66: // The function should not change the indexing of
the vector underfoot.
same comments apply here

http://codereview.appspot.com/178048/diff/2069/2071#newcode78
src/pkg/exp/vector/defs.go:78: func (p *StringVector) Do(f func(elem
interface{})) {
same comments apply here

http://codereview.appspot.com/178048/diff/2069/2077
File src/pkg/exp/vector/vector.go (right):

http://codereview.appspot.com/178048/diff/2069/2077#newcode1
src/pkg/exp/vector/vector.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
It would be nice to see the diffs relative to the src/pkg/container/vector again
- this file should be fairly close

http://codereview.appspot.com/178048/diff/2069/2077#newcode55
src/pkg/exp/vector/vector.go:55: // Resize adds the respective zero values for
the additional elements. The capacity parameter is ignored unless the
should probably break the line here and reformat the rest

http://codereview.appspot.com/178048/diff/2069/2078
File src/pkg/exp/vector/vector_test.go (right):

http://codereview.appspot.com/178048/diff/2069/2078#newcode1
src/pkg/exp/vector/vector_test.go:1: // Copyright 2009 The Go Authors. All
rights reserved.
I think it should be possible now to generate the different versions for this
file as well.

[jan_mercl, 2009-12-18 18:41:08]

New iteration available, PTAL.

bflm

http://codereview.appspot.com/178048/diff/2069/2070
File src/pkg/exp/vector/Makefile (right):

http://codereview.appspot.com/178048/diff/2069/2070#newcode14
src/pkg/exp/vector/Makefile:14: intvector.go: vector.go
On 2009/12/17 23:14:19, gri wrote:
> Note that this may cause this rule to be invoked (say after a fresh
installation
> if the time stamps are off by a ms) before gofmt exist and thus won't work.
> Probably need a separate generate rule which should be ok since the generated
> files are checked in anyway (for the above reason).

Done.

http://codereview.appspot.com/178048/diff/2069/2071
File src/pkg/exp/vector/defs.go (right):

http://codereview.appspot.com/178048/diff/2069/2071#newcode5
src/pkg/exp/vector/defs.go:5: // The vector package implements a container for
managing sequences
On 2009/12/17 23:14:19, gri wrote:
> s/a container/containers/
> 
> (there's three kinds)

Done.

http://codereview.appspot.com/178048/diff/2069/2071#newcode55
src/pkg/exp/vector/defs.go:55: // The function should not change the indexing of
the vector underfoot.
On 2009/12/17 23:14:19, gri wrote:
> Instead of the 2nd line write:
> 
> The behavior of Do is undefined if f changes *p.

Done.

http://codereview.appspot.com/178048/diff/2069/2071#newcode59
src/pkg/exp/vector/defs.go:59: for i := 0; i < len(a); i++ {
On 2009/12/17 23:14:19, gri wrote:
> actually, you should be able to use range here now:
> 
> for _, e := range *p {
>    f(e)
> }

Done.

http://codereview.appspot.com/178048/diff/2069/2071#newcode60
src/pkg/exp/vector/defs.go:60: f(a[i]) // not too safe if f changes the Vector
On 2009/12/17 23:14:19, gri wrote:
> instead of having this comment, document the interface

Done.

http://codereview.appspot.com/178048/diff/2069/2071#newcode66
src/pkg/exp/vector/defs.go:66: // The function should not change the indexing of
the vector underfoot.
On 2009/12/17 23:14:19, gri wrote:
> same comments apply here

Done.

http://codereview.appspot.com/178048/diff/2069/2071#newcode78
src/pkg/exp/vector/defs.go:78: func (p *StringVector) Do(f func(elem
interface{})) {
On 2009/12/17 23:14:19, gri wrote:
> same comments apply here

Done.

http://codereview.appspot.com/178048/diff/2069/2077
File src/pkg/exp/vector/vector.go (right):

http://codereview.appspot.com/178048/diff/2069/2077#newcode1
src/pkg/exp/vector/vector.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
On 2009/12/17 23:14:19, gri wrote:
> It would be nice to see the diffs relative to the src/pkg/container/vector
again
> - this file should be fairly close

Looked at http://codereview.appspot.com/178048/diff2/1:2069/2077 and can confirm
it is as close as I see possible.

Done.

http://codereview.appspot.com/178048/diff/2069/2077#newcode55
src/pkg/exp/vector/vector.go:55: // Resize adds the respective zero values for
the additional elements. The capacity parameter is ignored unless the
On 2009/12/17 23:14:19, gri wrote:
> should probably break the line here and reformat the rest

Done.

http://codereview.appspot.com/178048/diff/2069/2078
File src/pkg/exp/vector/vector_test.go (right):

http://codereview.appspot.com/178048/diff/2069/2078#newcode1
src/pkg/exp/vector/vector_test.go:1: // Copyright 2009 The Go Authors. All
rights reserved.
On 2009/12/17 23:14:19, gri wrote:
> I think it should be possible now to generate the different versions for this
> file as well.

Done.

[gri, 2009-12-18 21:41:12]

Close. A few more issues.

For the code to actually be built, you need to add exp/vector to pkg/Makefile .

You need to fill out a CLA form (see
http://golang.org/doc/contribute.html#copyright for details) so we can accept
your code.

- gri

http://codereview.appspot.com/178048/diff/2093/2095
File src/pkg/exp/vector/defs.go (right):

http://codereview.appspot.com/178048/diff/2093/2095#newcode29
src/pkg/exp/vector/defs.go:29: const bootstrap = 8
call this initialSize - says more then bootstrap

http://codereview.appspot.com/178048/diff/2093/2098
File src/pkg/exp/vector/nogen_test.go (right):

http://codereview.appspot.com/178048/diff/2093/2098#newcode21
src/pkg/exp/vector/nogen_test.go:21: func int2Value(x int) int { return x }
ok to have all the int2... grouped together without empty line, followed by an
empty line and then all elem2.... functions, etc. Will make this easier to read,
and gofmt will align the function bodies.

http://codereview.appspot.com/178048/diff/2093/2098#newcode39
src/pkg/exp/vector/nogen_test.go:39: func intf2Value(x interface{}) int { return
x.(int) }
intf2? what's the f for?

http://codereview.appspot.com/178048/diff/2093/2100
File src/pkg/exp/vector/nums.sh (right):

http://codereview.appspot.com/178048/diff/2093/2100#newcode6
src/pkg/exp/vector/nums.sh:6: cat /proc/cpuinfo | grep -m 1 'model name'
/proc/cpuinfo doesn't exist on OS X - leave away, or perhaps use 'uname -a'
(doesn't quite provide the same information)

[jan_mercl, 2009-12-19 08:47:49]

Iteration++, PTAL.

As this is presumably the issue finalization, I would like to thank to all
involved for your valuable help and the patience with my initial mercurial
trouble making.

bflm

http://codereview.appspot.com/178048/diff/2093/2095
File src/pkg/exp/vector/defs.go (right):

http://codereview.appspot.com/178048/diff/2093/2095#newcode29
src/pkg/exp/vector/defs.go:29: const bootstrap = 8
On 2009/12/18 21:41:12, gri wrote:
> call this initialSize - says more then bootstrap

Done.

http://codereview.appspot.com/178048/diff/2093/2098
File src/pkg/exp/vector/nogen_test.go (right):

http://codereview.appspot.com/178048/diff/2093/2098#newcode21
src/pkg/exp/vector/nogen_test.go:21: func int2Value(x int) int { return x }
On 2009/12/18 21:41:12, gri wrote:
> ok to have all the int2... grouped together without empty line, followed by an
> empty line and then all elem2.... functions, etc. Will make this easier to
read,
> and gofmt will align the function bodies.

Done.

http://codereview.appspot.com/178048/diff/2093/2098#newcode39
src/pkg/exp/vector/nogen_test.go:39: func intf2Value(x interface{}) int { return
x.(int) }
On 2009/12/18 21:41:12, gri wrote:
> intf2? what's the f for?

interface{} to ...

Could have been interfaced2* or any2* perhaps. No problem to change it but for
now I'm assuming that it can stay as it is. Otherwise please suggest the prefix.

Done.

http://codereview.appspot.com/178048/diff/2093/2100
File src/pkg/exp/vector/nums.sh (right):

http://codereview.appspot.com/178048/diff/2093/2100#newcode6
src/pkg/exp/vector/nums.sh:6: cat /proc/cpuinfo | grep -m 1 'model name'
On 2009/12/18 21:41:12, gri wrote:
> /proc/cpuinfo doesn't exist on OS X - leave away, or perhaps use 'uname -a'
> (doesn't quite provide the same information)

Dropped.

Done.

[gri, 2009-12-21 22:16:37]

LGTM.

Thanks for doing this.
- Robert

[gri, 2009-12-21 22:34:58]

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

Experimental alternative implementation of the vector package

R=gri
CC=rsc
http://codereview.appspot.com/178048

Committer: Robert Griesemer <gri@golang.org>