code review 181041: Replace container/vector with exp/vector (faster).

Replace container/vector with exp/vector (faster).

Manual changes to the following files:
src/pkg/Makefile
src/pkg/exp/vector/Makefile (now: src/pkg/container/vector/Makefile)

[gri, 2009-12-23 00:43:23]

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

I'd like you to review the following change.

[r2, 2009-12-23 00:46:44]

what is nums.sh?

i can't parse this CL. there are no diffs in the files.

-rob

On Dec 23, 2009, at 11:43 AM, gri@golang.org wrote:

> Reviewers: rsc, r,
>
> Message:
> Hello rsc, r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review the following change.
>
>
> Description:
> Replace container/vector with exp/vector (faster).
>
> Manual changes to the following files:
> src/pkg/Makefile
> src/pkg/exp/vector/Makefile (now: src/pkg/container/vector/Makefile)
>
> Please review this at http://codereview.appspot.com/181041
>
> Affected files:
>  M src/pkg/Makefile
>  M src/pkg/container/vector/Makefile
>  M src/pkg/container/vector/defs.go
>  M src/pkg/container/vector/intvector.go
>  M src/pkg/container/vector/intvector_test.go
>  M src/pkg/container/vector/nogen_test.go
>  M src/pkg/container/vector/numbers_test.go
>  M src/pkg/container/vector/nums.sh
>  M src/pkg/container/vector/stringvector.go
>  M src/pkg/container/vector/stringvector_test.go
>  M src/pkg/container/vector/vector.go
>  M src/pkg/container/vector/vector_test.go
>
>
> Index: src/pkg/Makefile
> ===================================================================
> --- a/src/pkg/Makefile
> +++ b/src/pkg/Makefile
> @@ -57,7 +57,6 @@
> 	exp/exception\
> 	exp/iterable\
> 	exp/parser\
> -	exp/vector\
> 	expvar\
> 	flag\
> 	fmt\
> Index: src/pkg/container/vector/Makefile
> ===================================================================
> rename from src/pkg/exp/vector/Makefile
> rename to src/pkg/container/vector/Makefile
> --- a/src/pkg/exp/vector/Makefile
> +++ b/src/pkg/container/vector/Makefile
> @@ -4,7 +4,7 @@
>
> include ../../../Make.$(GOARCH)
>
> -TARG=exp/vector
> +TARG=container/vector
> GOFILES=\
> 	defs.go\
> 	intvector.go\
> Index: src/pkg/container/vector/defs.go
> ===================================================================
> rename from src/pkg/exp/vector/defs.go
> rename to src/pkg/container/vector/defs.go
> Index: src/pkg/container/vector/intvector.go
> ===================================================================
> rename from src/pkg/exp/vector/intvector.go
> rename to src/pkg/container/vector/intvector.go
> Index: src/pkg/container/vector/intvector_test.go
> ===================================================================
> rename from src/pkg/exp/vector/intvector_test.go
> rename to src/pkg/container/vector/intvector_test.go
> Index: src/pkg/container/vector/nogen_test.go
> ===================================================================
> rename from src/pkg/exp/vector/nogen_test.go
> rename to src/pkg/container/vector/nogen_test.go
> Index: src/pkg/container/vector/numbers_test.go
> ===================================================================
> rename from src/pkg/exp/vector/numbers_test.go
> rename to src/pkg/container/vector/numbers_test.go
> Index: src/pkg/container/vector/nums.sh
> ===================================================================
> rename from src/pkg/exp/vector/nums.sh
> rename to src/pkg/container/vector/nums.sh
> Index: src/pkg/container/vector/stringvector.go
> ===================================================================
> rename from src/pkg/exp/vector/stringvector.go
> rename to src/pkg/container/vector/stringvector.go
> Index: src/pkg/container/vector/stringvector_test.go
> ===================================================================
> rename from src/pkg/exp/vector/stringvector_test.go
> rename to src/pkg/container/vector/stringvector_test.go
> Index: src/pkg/container/vector/vector.go
> ===================================================================
> rename from src/pkg/exp/vector/vector.go
> rename to src/pkg/container/vector/vector.go
> Index: src/pkg/container/vector/vector_test.go
> ===================================================================
> rename from src/pkg/exp/vector/vector_test.go
> rename to src/pkg/container/vector/vector_test.go
>
>

[gri, 2009-12-23 00:54:37]

On Tue, Dec 22, 2009 at 4:46 PM, Rob 'Commander' Pike <r@google.com> wrote:

> what is nums.sh?
>

just invokes gotest with some test parameters


>
> i can't parse this CL. there are no diffs in the files.
>

There are no diffs. It's a hg mv from src/pkg/exp/vector to
src/pkg/container/vector. I did an hg rm of the old container/vector files
but it looks like hg just silently "optimized" that away. For each
container/vector file there is an equivalent new file, so we don't see any
deletions (that's how I interpret it).

The new vector code provides exactly the same functionality as before, but
now also allows len() and cap() to be used directly. All tests run. The
intvector and stringvector code is completely generated from vector.go, same
with the respective tests.

Comparison of vector.go with the old version shows only minimal changes
related to the move from a struct to a []interface{} for the vector
implementation.

No strong feelings if you don't like this quite yet, but it seems to me that
we should not have multiple implementations.
- Robert


> -rob
>
>
> On Dec 23, 2009, at 11:43 AM, gri@golang.org wrote:
>
>  Reviewers: rsc, r,
>>
>> Message:
>> Hello rsc, r (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review the following change.
>>
>>
>> Description:
>> Replace container/vector with exp/vector (faster).
>>
>> Manual changes to the following files:
>> src/pkg/Makefile
>> src/pkg/exp/vector/Makefile (now: src/pkg/container/vector/Makefile)
>>
>> Please review this at http://codereview.appspot.com/181041
>>
>> Affected files:
>>  M src/pkg/Makefile
>>  M src/pkg/container/vector/Makefile
>>  M src/pkg/container/vector/defs.go
>>  M src/pkg/container/vector/intvector.go
>>  M src/pkg/container/vector/intvector_test.go
>>  M src/pkg/container/vector/nogen_test.go
>>  M src/pkg/container/vector/numbers_test.go
>>  M src/pkg/container/vector/nums.sh
>>  M src/pkg/container/vector/stringvector.go
>>  M src/pkg/container/vector/stringvector_test.go
>>  M src/pkg/container/vector/vector.go
>>  M src/pkg/container/vector/vector_test.go
>>
>>
>> Index: src/pkg/Makefile
>> ===================================================================
>> --- a/src/pkg/Makefile
>> +++ b/src/pkg/Makefile
>> @@ -57,7 +57,6 @@
>>        exp/exception\
>>        exp/iterable\
>>        exp/parser\
>> -       exp/vector\
>>        expvar\
>>        flag\
>>        fmt\
>> Index: src/pkg/container/vector/Makefile
>> ===================================================================
>> rename from src/pkg/exp/vector/Makefile
>> rename to src/pkg/container/vector/Makefile
>> --- a/src/pkg/exp/vector/Makefile
>> +++ b/src/pkg/container/vector/Makefile
>> @@ -4,7 +4,7 @@
>>
>> include ../../../Make.$(GOARCH)
>>
>> -TARG=exp/vector
>> +TARG=container/vector
>> GOFILES=\
>>        defs.go\
>>        intvector.go\
>> Index: src/pkg/container/vector/defs.go
>> ===================================================================
>> rename from src/pkg/exp/vector/defs.go
>> rename to src/pkg/container/vector/defs.go
>> Index: src/pkg/container/vector/intvector.go
>> ===================================================================
>> rename from src/pkg/exp/vector/intvector.go
>> rename to src/pkg/container/vector/intvector.go
>> Index: src/pkg/container/vector/intvector_test.go
>> ===================================================================
>> rename from src/pkg/exp/vector/intvector_test.go
>> rename to src/pkg/container/vector/intvector_test.go
>> Index: src/pkg/container/vector/nogen_test.go
>> ===================================================================
>> rename from src/pkg/exp/vector/nogen_test.go
>> rename to src/pkg/container/vector/nogen_test.go
>> Index: src/pkg/container/vector/numbers_test.go
>> ===================================================================
>> rename from src/pkg/exp/vector/numbers_test.go
>> rename to src/pkg/container/vector/numbers_test.go
>> Index: src/pkg/container/vector/nums.sh
>> ===================================================================
>> rename from src/pkg/exp/vector/nums.sh
>> rename to src/pkg/container/vector/nums.sh
>> Index: src/pkg/container/vector/stringvector.go
>> ===================================================================
>> rename from src/pkg/exp/vector/stringvector.go
>> rename to src/pkg/container/vector/stringvector.go
>> Index: src/pkg/container/vector/stringvector_test.go
>> ===================================================================
>> rename from src/pkg/exp/vector/stringvector_test.go
>> rename to src/pkg/container/vector/stringvector_test.go
>> Index: src/pkg/container/vector/vector.go
>> ===================================================================
>> rename from src/pkg/exp/vector/vector.go
>> rename to src/pkg/container/vector/vector.go
>> Index: src/pkg/container/vector/vector_test.go
>> ===================================================================
>> rename from src/pkg/exp/vector/vector_test.go
>> rename to src/pkg/container/vector/vector_test.go
>>
>>
>>
>

[r2, 2009-12-23 00:59:35]

I think I understand what I'm seeing now.

Where is the tool that generated the int vectors etc.?

I don't understand why defs.go defines IntVector etc. or why Do is  
singled out for appearance there.

The contents of nums.sh should just be an entry in the Makefile.

-rob

[r2, 2009-12-23 01:00:55]

Also, the nums business is poorly named and poorly documented.

-rob

[gri, 2009-12-23 01:06:00]

On Tue, Dec 22, 2009 at 4:59 PM, Rob 'Commander' Pike <r@google.com> wrote:

> I think I understand what I'm seeing now.
>
> Where is the tool that generated the int vectors etc.?
>

it's gofmt. It's in the makefile. The generated version is checked in
because it needs to be there for bootstrap.

>
> I don't understand why defs.go defines IntVector etc. or why Do is singled
> out for appearance there.
>
> The contents of nums.sh should just be an entry in the Makefile.
>
Agreed.

- Robert

>
> -rob
>
>

[gri, 2009-12-23 01:18:02]

PTAL.

- removed nums.sh
- added new target "bench" to Makefile

Note: The renaming in the Makefile has names of the form *Exp -> *Exp . The
Exp should go away eventually (separate CL). My understanding is that was
needed because these functions are global and conflicted with the respective
functions in container/vector when both were imported (this is due to the
fact that package namespaces are not yet fully independent if the path is
different but the package names are the same).

- Robert

On Tue, Dec 22, 2009 at 5:05 PM, Robert Griesemer <gri@golang.org> wrote:

> On Tue, Dec 22, 2009 at 4:59 PM, Rob 'Commander' Pike <r@google.com>wrote:
>
>> I think I understand what I'm seeing now.
>>
>> Where is the tool that generated the int vectors etc.?
>>
>
> it's gofmt. It's in the makefile. The generated version is checked in
> because it needs to be there for bootstrap.
>
>>
>> I don't understand why defs.go defines IntVector etc. or why Do is singled
>> out for appearance there.
>>
>> The contents of nums.sh should just be an entry in the Makefile.
>>
> Agreed.
>
> - Robert
>
>>
>> -rob
>>
>>
>

[r2, 2009-12-23 01:59:06]

upload?

[r2, 2009-12-23 01:59:35]

(nums.sh is still in the cl as shown by codereview)

[gri, 2009-12-23 02:08:44]

I did an hg rm of that file (again) and uploaded again. Don't know why it's
still there. It does show the changes to the local Makefile.
- Robert

On Tue, Dec 22, 2009 at 5:59 PM, Rob 'Commander' Pike <r@google.com> wrote:

> (nums.sh is still in the cl as shown by codereview)
>

[r2, 2009-12-23 02:21:51]

LGTM

[gri, 2009-12-23 02:25:33]

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

Replace container/vector with exp/vector (faster).

Manual changes to the following files:
src/pkg/Makefile
src/pkg/exp/vector/Makefile (now: src/pkg/container/vector/Makefile)

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

[rsc, 2009-12-23 15:09:44]

It looks like nums.sh got committed but is unused.

[gri, 2009-12-23 21:10:22]

thx. see CL 181049
- robert

On Wed, Dec 23, 2009 at 7:09 AM, Russ Cox <rsc@golang.org> wrote:

> It looks like nums.sh got committed but is unused.
>