PTAL
https://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.goFile src/pkg/reflect/value.go (right):
https://codereview.appspot.com/6554067/diff/11/src/pkg/reflect/value.go#newcode627src/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#newcode727src/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.goFile src/pkg/reflect/all_test.go (right):
https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/all_test.go#newcode1428src/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.goFile src/pkg/reflect/example_test.go (right):
https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/example_test.go#newcode28src/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.goFile src/pkg/reflect/makefunc.go (right):
https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/makefunc.go#newcode30src/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#newcode74src/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#newcode131src/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.goFile src/pkg/reflect/value.go (right):
https://codereview.appspot.com/6554067/diff/9003/src/pkg/reflect/value.go#newcode562src/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#newcode600src/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.
https://codereview.appspot.com/6554067/
