code review 3529042: reflect: add Append and AppendSlice functions.

reflect: add Append and AppendSlice functions.

[nigeltao, 2010-12-12 11:44:04]

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

I'd like you to review this change.

[nigeltao_gnome, 2010-12-12 11:52:15]

I think reflect.Append should take ...Value and not ...interface{}.
Let me know if you disagree.

Also, like the inbuilt append and copy functions, reflect.Append deals
only with slices and not arrays (partly because it must return a
slice, and if the input was of array type then I could not derive the
corresponding slice type). In comparison, reflect.Copy (formerly known
as reflect.ArrayCopy, possibly dating from before copy was an inbuilt
function) takes an ArrayOrSliceValue.

Should I cook up a separate CL to make reflect.Copy just take a
*SliceValue, and eliminate the ArrayOrSlice{Type,Value} types
altogether?

[rog, 2010-12-12 11:55:43]

On 12 December 2010 11:52, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> I think reflect.Append should take ...Value and not ...interface{}.
> Let me know if you disagree.
>
> Also, like the inbuilt append and copy functions, reflect.Append deals
> only with slices and not arrays (partly because it must return a
> slice, and if the input was of array type then I could not derive the
> corresponding slice type). In comparison, reflect.Copy (formerly known
> as reflect.ArrayCopy, possibly dating from before copy was an inbuilt
> function) takes an ArrayOrSliceValue.
>
> Should I cook up a separate CL to make reflect.Copy just take a
> *SliceValue, and eliminate the ArrayOrSlice{Type,Value} types
> altogether?

this would be fine if there was someway of creating a slice from an array;
for instance niemeyer on golang-nuts posted a reasonable way of doing this.

[nigeltao_gnome, 2010-12-12 12:05:48]

I'm not sure if Gustavo Niemeyer's patch is 100% correct. He might be
able to generate []int from [10]int provided that his program has
already referenced the []int type, but I'm not sure if it can generate
[]foo from [10]foo if the program otherwise doesn't explicitly mention
[]foo. I'm not totally sure how the runtime works, though, so I'll let
someone like Rob reply to that thread.

Even if it was technically possible, I'd still prefer to remove
ArrayOrSliceValue from reflect.Copy if it would make it consistent
with the inbuilt copy function.

On 12/12/2010, roger peppe <rogpeppe@gmail.com> wrote:
> On 12 December 2010 11:52, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
>> I think reflect.Append should take ...Value and not ...interface{}.
>> Let me know if you disagree.
>>
>> Also, like the inbuilt append and copy functions, reflect.Append deals
>> only with slices and not arrays (partly because it must return a
>> slice, and if the input was of array type then I could not derive the
>> corresponding slice type). In comparison, reflect.Copy (formerly known
>> as reflect.ArrayCopy, possibly dating from before copy was an inbuilt
>> function) takes an ArrayOrSliceValue.
>>
>> Should I cook up a separate CL to make reflect.Copy just take a
>> *SliceValue, and eliminate the ArrayOrSlice{Type,Value} types
>> altogether?
>
> this would be fine if there was someway of creating a slice from an array;
> for instance niemeyer on golang-nuts posted a reasonable way of doing this.
>

[rog, 2010-12-12 12:48:57]

i wouldn't want to lose the ability to copy from arrays with reflection.

On 12 December 2010 12:05, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> I'm not sure if Gustavo Niemeyer's patch is 100% correct. He might be
> able to generate []int from [10]int provided that his program has
> already referenced the []int type, but I'm not sure if it can generate
> []foo from [10]foo if the program otherwise doesn't explicitly mention
> []foo. I'm not totally sure how the runtime works, though, so I'll let
> someone like Rob reply to that thread.
>
> Even if it was technically possible, I'd still prefer to remove
> ArrayOrSliceValue from reflect.Copy if it would make it consistent
> with the inbuilt copy function.
>
> On 12/12/2010, roger peppe <rogpeppe@gmail.com> wrote:
>> On 12 December 2010 11:52, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
>>> I think reflect.Append should take ...Value and not ...interface{}.
>>> Let me know if you disagree.
>>>
>>> Also, like the inbuilt append and copy functions, reflect.Append deals
>>> only with slices and not arrays (partly because it must return a
>>> slice, and if the input was of array type then I could not derive the
>>> corresponding slice type). In comparison, reflect.Copy (formerly known
>>> as reflect.ArrayCopy, possibly dating from before copy was an inbuilt
>>> function) takes an ArrayOrSliceValue.
>>>
>>> Should I cook up a separate CL to make reflect.Copy just take a
>>> *SliceValue, and eliminate the ArrayOrSlice{Type,Value} types
>>> altogether?
>>
>> this would be fine if there was someway of creating a slice from an array;
>> for instance niemeyer on golang-nuts posted a reasonable way of doing this.
>>
>

[niemeyer, 2010-12-12 13:32:14]

> I'm not sure if Gustavo Niemeyer's patch is 100% correct. He might be
> able to generate []int from [10]int provided that his program has
> already referenced the []int type, but I'm not sure if it can generate
> []foo from [10]foo if the program otherwise doesn't explicitly mention
> []foo. I'm not totally sure how the runtime works, though, so I'll let
> someone like Rob reply to that thread.

No need to wonder.  The patch is there, so just give it a try.

The whole thread seems tainted with the "can't create a new type".  No
new types are created at runtime. It enforces the dumping of the slice
type information for the given array, which will likely happen anyway
since slices are such a key part of dealing with arrays, but it doesn't
depend on them being used elsewhere because, effectively, it *ensures*
their type is available.

Again, I'll be happy to learn about any issues with this approach, but
would appreciate better input.

[niemeyer, 2010-12-12 13:35:41]

> No need to wonder.  The patch is there, so just give it a try.

Patch updated with a test using a never-touched slice:

http://pastebin.ubuntu.com/542669/

[r, 2010-12-12 17:31:26]

http://codereview.appspot.com/3529042/diff/4/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

http://codereview.appspot.com/3529042/diff/4/src/pkg/reflect/all_test.go#newc...
src/pkg/reflect/all_test.go:521: }
this and TestCopy should probably be table-driven. it's easy to do: given two
slices, compare the results of append() and Append().

[nigeltao_gnome, 2010-12-12 21:24:12]

On 13/12/2010, r@golang.org <r@golang.org> wrote:
>
http://codereview.appspot.com/3529042/diff/4/src/pkg/reflect/all_test.go#newc...
> src/pkg/reflect/all_test.go:521: }
> this and TestCopy should probably be table-driven. it's easy to do:
> given two slices, compare the results of append() and Append().

I've rewritten TestAppend to be table-driven, but I'm not sure if it's
better. It's certainly longer, mostly because append takes []int but
reflect.Append takes []Value and reflect.AppendSlice takes
*SliceValue. PTAL. If you like the table style then I'll also have a
look at TestCopy.

Also, any opinion on removing ArrayOrSliceValue and restricting
reflect.Copy to only work on slices?

[niemeyer, 2010-12-12 22:13:30]

> Also, any opinion on removing ArrayOrSliceValue and restricting
> reflect.Copy to only work on slices?

With the change submitted at:

http://codereview.appspot.com/3608041/

you can easily make it work well with ArrayOrSliceValue rather than
restricting it to slices.

[nigeltao_gnome, 2010-12-12 22:28:30]

On 13 December 2010 09:13,  <n13m3y3r@gmail.com> wrote:
>> Also, any opinion on removing ArrayOrSliceValue and restricting
>> reflect.Copy to only work on slices?
>
> With the change submitted at:
>
> http://codereview.appspot.com/3608041/
>
> you can easily make it work well with ArrayOrSliceValue rather than
> restricting it to slices.

Yes, I know this, but I'd still rather remove ArrayOrSliceValue and
restrict reflect.Append and reflect.Copy to match the built-in append
and copy.

[nigeltao_gnome, 2010-12-12 22:29:11]

On 13 December 2010 08:24, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> Also, any opinion on removing ArrayOrSliceValue and restricting
> reflect.Copy to only work on slices?

In case this isn't obvious, I'm seeking Rob's opinion.

[niemeyer, 2010-12-12 22:42:21]

> Yes, I know this, but I'd still rather remove ArrayOrSliceValue and
> restrict reflect.Append and reflect.Copy to match the built-in append
> and copy.

I see.. this sounds sensible.

[nigeltao_gnome, 2010-12-14 01:04:05]

On 13 December 2010 08:24, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 13/12/2010, r@golang.org <r@golang.org> wrote:
>>
http://codereview.appspot.com/3529042/diff/4/src/pkg/reflect/all_test.go#newc...
>> src/pkg/reflect/all_test.go:521: }
>> this and TestCopy should probably be table-driven. it's easy to do:
>> given two slices, compare the results of append() and Append().
>
> I've rewritten TestAppend to be table-driven, but I'm not sure if it's
> better. It's certainly longer, mostly because append takes []int but
> reflect.Append takes []Value and reflect.AppendSlice takes
> *SliceValue. PTAL. If you like the table style then I'll also have a
> look at TestCopy.
>
> Also, any opinion on removing ArrayOrSliceValue and restricting
> reflect.Copy to only work on slices?

Ping.

[r, 2010-12-14 04:00:27]

i don't see why ArrayOrSliceValue should disappear. it's useful in other
contexts. and as far as reflect.Copy goes, it's hard to create a slice from an
array, so leave it alone.

http://codereview.appspot.com/3529042/diff/12001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

http://codereview.appspot.com/3529042/diff/12001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:509: }
tradition keeps these outside.   also i don't believe you need to name the type.
var appendTests = []struct {xxx} { ... }

[nigeltao_gnome, 2010-12-14 12:21:12]

On 14/12/2010, r@golang.org <r@golang.org> wrote:
> i don't see why ArrayOrSliceValue should disappear. it's useful in other
> contexts. and as far as reflect.Copy goes, it's hard to create a slice
> from an array, so leave it alone.

OK, it can stay.


> http://codereview.appspot.com/3529042/diff/12001/src/pkg/reflect/all_test.go
> File src/pkg/reflect/all_test.go (right):
>
>
http://codereview.appspot.com/3529042/diff/12001/src/pkg/reflect/all_test.go#...
> src/pkg/reflect/all_test.go:509: }
> tradition keeps these outside.   also i don't believe you need to name
> the type.
> var appendTests = []struct {xxx} { ... }

Done. PTAL.

I had a look at TestCopy but it's not obvious to me how to make it a
'data driven test' since it really only has one test case.

It also seems odd how it creates pointers to slices only to
dereference them again via reflection, but it probably made sense in
the distant past. I'll clean that up in a separate CL.

[r, 2010-12-14 19:29:10]

LGTM

[nigeltao, 2010-12-14 21:50:03]

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

reflect: add Append and AppendSlice functions.

R=r, nigeltao_gnome, rog, niemeyer
CC=golang-dev
http://codereview.appspot.com/3529042