code review 6554067: reflect: add MakeFunc (API CHANGE)

reflect: add MakeFunc (API CHANGE)

Fixes issue 1765.

[rsc, 2012-09-22 14:17:14]

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

I'd like you to review this change to
https://code.google.com/p/go/

[DMorsing, 2012-09-22 16:53:31]

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode651
src/pkg/reflect/value.go:651: switch runtime.GOARCH {
Could this be moved into a conditionally compiled file?

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode698
src/pkg/reflect/value.go:698: var amd64CallStub = []byte{
Conditional compilation again

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode712
src/pkg/reflect/value.go:712: var _386CallStub = []byte{
ditto.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode726
src/pkg/reflect/value.go:726: var armCallStub = []uintptr{
ditto.

[rsc, 2012-09-22 16:58:30]

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode651
src/pkg/reflect/value.go:651: switch runtime.GOARCH {
On 2012/09/22 16:53:31, DMorsing wrote:
> Could this be moved into a conditionally compiled file?

I haven't double-checked, but since this is a constant switch with constant
cases, the compiler should only emit code for the case that matters. Then the
linker will throw away the data mentioned by the other cases. If that's not the
case, I'd prefer to fix that than to create a few tiny files.

[minux1, 2012-09-22 17:48:12]

Sigh, one more place of dynamic code generation.
Do you have plan to remove dyn. code gen. in the future?

[bradfitzgoog, 2012-09-22 18:17:23]

Wow, nice surprise.

Fixes issue nnnn.
 On Sep 22, 2012 7:17 AM, <rsc@golang.org> wrote:

> Reviewers: iant, r,
>
> Message:
> Hello iant, r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> reflect: add MakeFunc (API CHANGE)
>
> Please review this at
http://codereview.appspot.com/**6554067/<http://codereview.appspot.com/6554067/>
>
> Affected files:
>   M src/pkg/reflect/all_test.go
>   M src/pkg/reflect/value.go
>
>
>

[r, 2012-09-22 18:44:19]

this is one of the ugliest CLs i've ever seen.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/asm_386.s
File src/pkg/reflect/asm_386.s (right):

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/asm_386.s#newcode5
src/pkg/reflect/asm_386.s:5: TEXT ·callStub(SB),7,$12
a word or two of documentation would be nice. to call this magic is to praise it
unduly.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode627
src/pkg/reflect/value.go:627: // results returned to the caller.
needs an example. it's hard to see what this does given only this explanation.
the swap example in the issue is a good one.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode686
src/pkg/reflect/value.go:686: //	JMP callStub
callStub isn't a good name: other than the vowels it shares no characters with
MakeFunc. why not makeFuncStub or something like that?  is it even a stub?

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode694
src/pkg/reflect/value.go:694: // Nothing ever returns to the allocated code.
i think this belongs in the asm files, and here you say:

// callStub is a callStub is an assembly function used by the code generated
// and returned from MakeFunc. See the assembly implementations for more
// information.
func callStub() // except give it a name that helps

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode708
src/pkg/reflect/value.go:708: 0xff, 0xe2,
this is disgusting. it may even be worse than an asm call. at least move it to
the files with the other assembler code

[r, 2012-09-22 18:45:34]

Minux is right about code generation, but given the variable type I
don't see how to avoid that. Worth thinking about more, though.

-rob

[rog, 2012-09-22 19:07:21]

On 22 September 2012 19:45, Rob Pike <r@golang.org> wrote:
> Minux is right about code generation, but given the variable type I
> don't see how to avoid that. Worth thinking about more, though.

I suspect that using a two-word representation of function
pointers would alleviate the need for runtime code generation
here as it does for closures.

Interesting new capability.

[rsc, 2012-09-22 23:23:55]

On Sat, Sep 22, 2012 at 1:48 PM, minux <minux.ma@gmail.com> wrote:
> Sigh, one more place of dynamic code generation.
> Do you have plan to remove dyn. code gen. in the future?

Yes, quite soon. This is no different than making closures; the same
solution will handle both.

Russ

[rsc, 2012-09-22 23:34:26]

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/asm_386.s
File src/pkg/reflect/asm_386.s (right):

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/asm_386.s#newcode5
src/pkg/reflect/asm_386.s:5: TEXT ·callStub(SB),7,$12
On 2012/09/22 18:44:19, r wrote:
> a word or two of documentation would be nice. to call this magic is to praise
it
> unduly.

I'm sorry this is the first file you saw. There are big comments in value.go but
I will add a comment here pointing there.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode627
src/pkg/reflect/value.go:627: // results returned to the caller.
On 2012/09/22 18:44:19, r wrote:
> needs an example.

Indeed. The swap from the issue is in example_test.go.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode686
src/pkg/reflect/value.go:686: //	JMP callStub
On 2012/09/22 18:44:19, r wrote:
> callStub isn't a good name: other than the vowels it shares no characters with
> MakeFunc. why not makeFuncStub or something like that?  is it even a stub? 

Will rename to makeFuncStub.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode694
src/pkg/reflect/value.go:694: // Nothing ever returns to the allocated code.
On 2012/09/22 18:44:19, r wrote:
> i think this belongs in the asm files, and here you say:
> 
> // callStub is a callStub is an assembly function used by the code generated
> // and returned from MakeFunc. See the assembly implementations for more
> // information.
> func callStub() // except give it a name that helps

I am happy to rename it, but I'd like to keep the comment here. All the logic
and explanation is in one place right now, and if I move the comment out I have
to duplicate it in three different files. I will add a pointer comment in the
assembly.

http://codereview.appspot.com/6554067/diff/3/src/pkg/reflect/value.go#newcode708
src/pkg/reflect/value.go:708: 0xff, 0xe2,
On 2012/09/22 18:44:19, r wrote:
> this is disgusting. 

It is, yes. It will go away when we remove dynamic code generation, both from
here and from closures. For what it's worth, this code is far *less* disgusting
than what's in src/pkg/runtime/closure*.c.

> it may even be worse than an asm call. at least move it to
> the files with the other assembler code

I don't think that would help. Here the bytes and the code using the bytes
(above in the switch in MakeFunc) are in the same file, so that you can match
them against each other. Writing in the assembly file I'd have to use DATA
statements, making the definitions even more awkward than they are now. At least
here I don't have to lay out a slice header by hand.

[rsc, 2012-09-22 23:40:27]

Hello iant@golang.org, r@golang.org, daniel.morsing@gmail.com,
minux.ma@gmail.com, bradfitz@google.com, rogpeppe@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2012-09-23 01:07:56]

is there urgent need for this? i don't have this itch to scratch.

if it cannot wait for improved representation of function values, please park it
somewhere self-contained.

on a different topic, i worry about the generic abuse this will encourage.

http://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go#newcod...
src/pkg/reflect/value.go:627: // results returned to the caller.
this still isn't clear because the words are hard to follow and because the
function is a small part of a longer story that is what we want to do. for
instance, there are slices involved but no indication about how the slices
relate in the multiple functions involved. i believe you really want an example
here. at least point to where an example can be found.

http://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go#newcod...
src/pkg/reflect/value.go:727: 0xe59f000c, // MOVW 0x14(PC), R0
can we at least put all this ugliness, including MakeFunc, into a separate file?
it's jarring, plus i believe it will all be rewritten if we change the
representation of function values. here we have hexadecimal instruction decodes
immediately before the clean, simple implementation of Cap.  worlds are
colliding.

[rsc, 2012-09-23 01:17:34]

On Sat, Sep 22, 2012 at 9:07 PM,  <r@golang.org> wrote:
> is there urgent need for this? i don't have this itch to scratch.
> if it cannot wait for improved representation of function values, please
> park it somewhere self-contained.

it will get cleaned up then, but i'd rather not wait until then.

> on a different topic, i worry about the generic abuse this will
> encourage.

i do too, but to some extent that's true of all of reflect. anything
can be abused.
i will move the ugly part to a new file.

russ

[rsc, 2012-09-23 01:24:11]

Hello iant@golang.org, r@golang.org, daniel.morsing@gmail.com,
minux.ma@gmail.com, bradfitz@google.com, rogpeppe@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[iant, 2012-09-23 05:10:54]

LGTM

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:1428: var f func(byte, int, byte, two, byte) (byte,
int, byte, two, byte)
It doesn't matter for gc, but for gccgo this test would be better if it used
some floating point values, as they are passed in different registers.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:74: }
default:
    panic("forgot to implement this")

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:131: 0xe28d2004, // MOVW $4(SP), R2
Is MOVW $4(SP), R2 really the 5a mnemonic for this instruction?  Conventionally
it is add r2, sp, #4.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:562: var in []Value
in := make([]Value, 0, len(ftyp.in))

[r, 2012-09-23 05:56:08]

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/example_test.go
File src/pkg/reflect/example_test.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/example_test...
src/pkg/reflect/example_test.go:28: reflect.ValueOf(fptr).Elem().Set(v)
this line needs explanation.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:30: func MakeFunc(typ Type, fn func([]Value)
[]Value) Value {
i think the type
func([]Value) []Value
should be given an exported name, maybe MakeFuncFunc but perhaps there's a
better one.
that would give this the signature

func MakeFunc(typ Type, fn MakeFuncFunc) Value
which is easier to read, with the slices tucked away. it also gives you a
declaration (the one for MakeFuncFunc) to explain what it is and why.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:600: panic("reflect: function created by MakeFunc
returned value obtained from unexported field")
can you dig out the function's name type for the message? i imagine when this
triggers it could be far removed from where the error was created.

[rsc, 2012-09-23 17:51:53]

PTAL

https://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go#newco...
src/pkg/reflect/value.go:627: // results returned to the caller.
On 2012/09/23 01:07:56, r wrote:
> this still isn't clear because the words are hard to follow and because the
> function is a small part of a longer story that is what we want to do. for
> instance, there are slices involved but no indication about how the slices
> relate in the multiple functions involved. i believe you really want an
example
> here. at least point to where an example can be found.

I agree that the existence of the example should be made clear, but how? In
godoc HTML output it will just say

 > Example

and you click on the > and there it is. But in the command-line output there's
no indication and no way to get to the example (an open bug). I am not sure what
to write that works in both contexts.

"See the example for ..."

https://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go#newco...
src/pkg/reflect/value.go:727: 0xe59f000c, // MOVW 0x14(PC), R0
On 2012/09/23 01:07:56, r wrote:
> can we at least put all this ugliness, including MakeFunc, into a separate
file?
> it's jarring, plus i believe it will all be rewritten if we change the
> representation of function values. here we have hexadecimal instruction
decodes
> immediately before the clean, simple implementation of Cap.  worlds are
> colliding.

Done.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:1428: var f func(byte, int, byte, two, byte) (byte,
int, byte, two, byte)
On 2012/09/23 05:10:54, iant wrote:
> It doesn't matter for gc, but for gccgo this test would be better if it used
> some floating point values, as they are passed in different registers.

Done.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/example_test.go
File src/pkg/reflect/example_test.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/example_test...
src/pkg/reflect/example_test.go:28: reflect.ValueOf(fptr).Elem().Set(v)
On 2012/09/23 05:56:08, r wrote:
> this line needs explanation.

Rewrote the code to be clearer, added explanation.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:30: func MakeFunc(typ Type, fn func([]Value)
[]Value) Value {
On 2012/09/23 05:56:08, r wrote:
> i think the type
> func([]Value) []Value
> should be given an exported name, maybe MakeFuncFunc but perhaps there's a
> better one.
> that would give this the signature
> 
> func MakeFunc(typ Type, fn MakeFuncFunc) Value
> which is easier to read, with the slices tucked away. it also gives you a
> declaration (the one for MakeFuncFunc) to explain what it is and why.

While I see the documentation benefit, this would introduce a name that has no
use in client code, so I'd prefer not to. (FuncMap is the obvious parallel, but
there clients can declare FuncMap literals.) Also the documentation would end up
in a different place on the page than the MakeFunc function, so it would require
finding two places instead of one. I think we can probably work to make the
documentation clear here. I certainly agree it's not there yet. 

I have attempted to rewrite it, and I also named the []Value in the func type:

// MakeFunc returns a new function of the given type.
// When invoked, that new function does the following:
//
//	- convert its arguments to a list of Values in.
//	- run out := fn(in).
//	- return the values listed in out.
//
// The Value.Call method allows the caller to
// invoke a typed function in terms of Values;
// in contrast, MakeFunc allows the caller to
// implement a typed function in terms of Values.
//
// See the example for <WHAT GOES HERE>?
//
func MakeFunc(typ Type, fn func(in []Value) (out []Value)) Value {

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:74: }
On 2012/09/23 05:10:54, iant wrote:
> default:
>     panic("forgot to implement this")

Done.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:131: 0xe28d2004, // MOVW $4(SP), R2
On 2012/09/23 05:10:54, iant wrote:
> Is MOVW $4(SP), R2 really the 5a mnemonic for this instruction? 
Conventionally
> it is add r2, sp, #4.

It is certainly a valid one; I don't know if it is the only one.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:562: var in []Value
On 2012/09/23 05:10:54, iant wrote:
> in := make([]Value, 0, len(ftyp.in))

Done.

https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:600: panic("reflect: function created by MakeFunc
returned value obtained from unexported field")
On 2012/09/23 05:56:08, r wrote:
> can you dig out the function's name type for the message? i imagine when this
> triggers it could be far removed from where the error was created.

Done.

[remyoudompheng, 2012-09-23 18:03:03]

How does it interact with variadic function types? The supplied func([]Value)
[]Value must behave like Value.Call or like Value.CallSlice?

I think it should be documented in some way.

[rsc, 2012-09-23 18:15:28]

On Sun, Sep 23, 2012 at 2:03 PM,  <remyoudompheng@gmail.com> wrote:
> How does it interact with variadic function types? The supplied
> func([]Value) []Value must behave like Value.Call or like
> Value.CallSlice?
>
> I think it should be documented in some way.

Done. The answer is that since fn is in effect the function body, it
sees a single argument of slice type holding all the variadic
parameters, just as in an ordinary function body.

[r, 2012-09-23 20:41:24]

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go
File src/pkg/reflect/example_test.go (right):

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go...
src/pkg/reflect/example_test.go:24: // into Values, calls swap, and then turns
swaps result slice
s/swaps/swap's/

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go...
src/pkg/reflect/example_test.go:39: // Make and call an int-valued swap
function.
the swap function isn't int-valued, its arguments are.

// Make and call a swap function for ints.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go...
src/pkg/reflect/example_test.go:44: // Make a float64-valued swap function.
// Make and call a swap function for float64s.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:18: // The code holds the same references.
which code? be clear.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:22: // code is the actual machine code invoked for
the function.
which function? be clear.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:31: //	- return the values listed in out.
'in' is a bad choice of name here. so is list.
- convert its arguments to a slice of Values, args.
- run results := fn(args)
- returns the results as a slice of Values, one per formal result.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:36: // a slice representing the variadic arguments,
like in the
s/like/as/

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:45: // See the example for <WHAT GOES HERE>?
// The Examples section of the documentation includes an illustration of
// how to use MakeFunc to build a swap function for different types.
(you might even be able to make a useful URL here - not sure)

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:47: func MakeFunc(typ Type, fn func(in []Value) (out
[]Value)) Value {
s/in/args/
s/out/results/

[r, 2012-09-23 20:42:26]

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:31: //	- return the values listed in out.
my mistake: in the last line, s/returns/return/

[rsc, 2012-09-24 19:59:15]

PTAL

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go
File src/pkg/reflect/example_test.go (right):

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go...
src/pkg/reflect/example_test.go:24: // into Values, calls swap, and then turns
swaps result slice
On 2012/09/23 20:41:25, r wrote:
> s/swaps/swap's/

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go...
src/pkg/reflect/example_test.go:39: // Make and call an int-valued swap
function.
On 2012/09/23 20:41:25, r wrote:
> the swap function isn't int-valued, its arguments are.
> 
> // Make and call a swap function for ints.

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/example_test.go...
src/pkg/reflect/example_test.go:44: // Make a float64-valued swap function.
On 2012/09/23 20:41:25, r wrote:
> // Make and call a swap function for float64s.

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:18: // The code holds the same references.
On 2012/09/23 20:41:25, r wrote:
> which code? be clear.

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:22: // code is the actual machine code invoked for
the function.
On 2012/09/23 20:41:25, r wrote:
> which function? be clear.

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:31: //	- return the values listed in out.
On 2012/09/23 20:41:25, r wrote:
> 'in' is a bad choice of name here. so is list.
> - convert its arguments to a slice of Values, args.
> - run results := fn(args)
> - returns the results as a slice of Values, one per formal result.

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:31: //	- return the values listed in out.
On 2012/09/23 20:41:25, r wrote:
> 'in' is a bad choice of name here. so is list.
> - convert its arguments to a slice of Values, args.
> - run results := fn(args)
> - returns the results as a slice of Values, one per formal result.

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:36: // a slice representing the variadic arguments,
like in the
On 2012/09/23 20:41:25, r wrote:
> s/like/as/

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:45: // See the example for <WHAT GOES HERE>?
On 2012/09/23 20:41:25, r wrote:
> // The Examples section of the documentation includes an illustration of
> // how to use MakeFunc to build a swap function for different types.
> (you might even be able to make a useful URL here - not sure)

Done.

http://codereview.appspot.com/6554067/diff/20/src/pkg/reflect/makefunc.go#new...
src/pkg/reflect/makefunc.go:47: func MakeFunc(typ Type, fn func(in []Value) (out
[]Value)) Value {
On 2012/09/23 20:41:25, r wrote:
> s/in/args/
> s/out/results/

Done.

[r, 2012-09-24 21:59:02]

comments almost there

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:27: // MakeFunc returns a new function of the given
type.
MakeFunc returns a Value representing a new function of the given Type
that wraps the function fn.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:28: // When invoked, that new function does the
following:
When invoked by Call, that new function...

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:30: //	- convert its arguments to a list of Values
in.
s/in/args/

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:32: //	- return the results as a slice of Values,
one per formal result.
these should be singular verbs to echo 'does'

converts
runs
returns

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:34: // The implementation fn can assume that the
input Value slice
s/input/argument/

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:38: // body of a variadic function. The output Value
slice returned by fn
s/output/result/

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:44: // implement a typed function in terms of
Values.
inconsistent formatting of line breaks

[rsc, 2012-09-24 22:07:59]

PTAL

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go
File src/pkg/reflect/makefunc.go (right):

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:27: // MakeFunc returns a new function of the given
type.
On 2012/09/24 21:59:02, r wrote:
> MakeFunc returns a Value representing a new function of the given Type
> that wraps the function fn.

Done.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:28: // When invoked, that new function does the
following:
On 2012/09/24 21:59:02, r wrote:
> When invoked by Call, that new function...

I used "When called". The new function is almost never invoked using Call. This
routine is the opposite of Call. You use it when you need to supply some
non-reflecty code with a function of a specific type that can be called directly
as an ordinary Go function.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:30: //	- convert its arguments to a list of Values
in.
On 2012/09/24 21:59:02, r wrote:
> s/in/args/

Done.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:32: //	- return the results as a slice of Values,
one per formal result.
On 2012/09/24 21:59:02, r wrote:
> these should be singular verbs to echo 'does'
> 
> converts
> runs
> returns

Done.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:34: // The implementation fn can assume that the
input Value slice
On 2012/09/24 21:59:02, r wrote:
> s/input/argument/

Done.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:38: // body of a variadic function. The output Value
slice returned by fn
On 2012/09/24 21:59:02, r wrote:
> s/output/result/

Done.

http://codereview.appspot.com/6554067/diff/12011/src/pkg/reflect/makefunc.go#...
src/pkg/reflect/makefunc.go:44: // implement a typed function in terms of
Values.
On 2012/09/24 21:59:02, r wrote:
> inconsistent formatting of line breaks

Done. (I'd been trying to make the parallelism clear, but even godoc in text
mode is going to reformat it.)

[r, 2012-09-24 22:34:33]

LGTM

[rsc, 2012-09-25 00:06:39]

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

reflect: add MakeFunc (API CHANGE)

Fixes issue 1765.

R=iant, r, daniel.morsing, minux.ma, bradfitz, rogpeppe, remyoudompheng
CC=golang-dev
http://codereview.appspot.com/6554067