code review 12895043: runtime: make SetFinalizer(x, f) accept any f for which...

runtime: make SetFinalizer(x, f) accept any f for which f(x) is valid

Originally the requirement was f(x) where f's argument is
exactly x's type.

CL 11858043 relaxed the requirement in a non-standard
way: f's argument must be exactly x's type or interface{}.

If we're going to relax the requirement, it should be done
in a way consistent with the rest of Go. This CL allows f's
argument to have any type for which x is assignable;
that's the same requirement the compiler would impose
if compiling f(x) directly.

Fixes issue 5368.

[rsc, 2013-08-13 21:30:29]

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

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

[bradfitz, 2013-08-13 21:46:35]

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:768: if(fint == obj.type) {
can't put this into an isassignable bool func?

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:1044: bool runtime·ifaceE2I2(InterfaceType*, Eface,
Iface*);
is this supposed to line up with above?

[peted, 2013-08-14 03:08:27]

On 2013/08/13 21:46:35, bradfitz wrote:
> https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc
> File src/pkg/runtime/malloc.goc (right):
> 
>
https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:768: if(fint == obj.type) {
> can't put this into an isassignable bool func?
> 
> https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/runtime.h
> File src/pkg/runtime/runtime.h (right):
> 
>
https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/runtime.h#n...
> src/pkg/runtime/runtime.h:1044: bool runtime·ifaceE2I2(InterfaceType*, Eface,
> Iface*);
> is this supposed to line up with above?

Looks great. Thank you for doing this Russ. My previous CL was about as deep
into the Runtime as I dared to go without diving lessons.

[dvyukov, 2013-08-14 09:27:48]

Other than my confusion with definitions, the code looks good

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:770: } else if(fint->kind == KindPtr && (fint->x ==
nil || fint->x->name == nil || obj.type->x == nil || obj.type->x->name == nil)
&& ((PtrType*)fint)->elem == ((PtrType*)obj.type)->elem) {
The spec uses "named type" to define several things, but I can not find what
"named type" is, all entries of "named type" are usages of the term...

I am also confused by "Two named types are identical if their type names
originate in the same TypeSpec". How can more than 1 type originate in a single
TypeSpec, TypeSpec can possibly define only 1 type...

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal.c
File src/pkg/runtime/mfinal.c (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal.c#ne...
src/pkg/runtime/mfinal.c:163: 
delete

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal_test.go
File src/pkg/runtime/mfinal_test.go (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:45: {func(x *int) interface{} { return (*Tint)(x)
}, func(v Tinter) { finalize((*int)(v.(*Tint))) }},
Should it work for:
v := new(struct{foo int})
SetFinalizer(v, func(p *struct{foo int}) {})
?
These are identical types, right?

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2341: } else {
please add comments above these 3 blocks with types of conversion they do
e.g. *T -> Eface -> Iface

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:2344: runtime·ifaceE2I2((InterfaceType*)f->fint, ef1,
(Iface*)frame);
since you throw above, you may also throw here

[rsc, 2013-08-14 14:37:38]

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc#...
src/pkg/runtime/malloc.goc:770: } else if(fint->kind == KindPtr && (fint->x ==
nil || fint->x->name == nil || obj.type->x == nil || obj.type->x->name == nil)
&& ((PtrType*)fint)->elem == ((PtrType*)obj.type)->elem) {
On 2013/08/14 09:27:49, dvyukov wrote:
> The spec uses "named type" to define several things, but I can not find what
> "named type" is, all entries of "named type" are usages of the term...

A named type is a type with a name. bool is a named type. *int is not. After
type T *int, T is a named type.

> I am also confused by "Two named types are identical if their type names
> originate in the same TypeSpec". How can more than 1 type originate in a
single
> TypeSpec, TypeSpec can possibly define only 1 type...

This means that if you have

{
1    type T int
2    var x T
3    var y T
4    _ = x==y
}

the two types referred to by T on line 2 and 3 are in fact identical because
they both refer to the type originating in the TypeSpec on line 1. So the
comparison on line 4 is legal.

It also means that


{
1   type T int
2   var x T
    {
    3   type T int
    4   var y T
    5   _ = x==y
    }
}

The types on lines 2 and 4 refer to different typespecs (lines 1 and 3) so the
comparison on line 5 is not legal.

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal_test.go
File src/pkg/runtime/mfinal_test.go (right):

https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal_test...
src/pkg/runtime/mfinal_test.go:45: {func(x *int) interface{} { return (*Tint)(x)
}, func(v Tinter) { finalize((*int)(v.(*Tint))) }},
On 2013/08/14 09:27:49, dvyukov wrote:
> Should it work for:
> v := new(struct{foo int})
> SetFinalizer(v, func(p *struct{foo int}) {})
> ?
> These are identical types, right?

Yes, that works. I will add a test.

[dvyukov, 2013-08-14 16:35:00]

On 2013/08/14 14:37:38, rsc wrote:
> https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc
> File src/pkg/runtime/malloc.goc (right):
> 
>
https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/malloc.goc#...
> src/pkg/runtime/malloc.goc:770: } else if(fint->kind == KindPtr && (fint->x ==
> nil || fint->x->name == nil || obj.type->x == nil || obj.type->x->name == nil)
> && ((PtrType*)fint)->elem == ((PtrType*)obj.type)->elem) {
> On 2013/08/14 09:27:49, dvyukov wrote:
> > The spec uses "named type" to define several things, but I can not find what
> > "named type" is, all entries of "named type" are usages of the term...
> 
> A named type is a type with a name. bool is a named type. *int is not. After
> type T *int, T is a named type.


Before I realized that "*int" may be an "unnamed" type, I honestly thought that
"bool" maybe an "unnamed" type, because what else then? Once you know it, it's
obvious.


> > I am also confused by "Two named types are identical if their type names
> > originate in the same TypeSpec". How can more than 1 type originate in a
> single
> > TypeSpec, TypeSpec can possibly define only 1 type...
> 
> This means that if you have
> 
> {
> 1    type T int
> 2    var x T
> 3    var y T
> 4    _ = x==y
> }
> 
> the two types referred to by T on line 2 and 3 are in fact identical because


I would expect that this code fragment ever contains only 1 type.
In what TypeSpec all the bool types (are there actually multiple bool types?)
originate?



> they both refer to the type originating in the TypeSpec on line 1. So the
> comparison on line 4 is legal.
> 
> It also means that
> 
> 
> {
> 1   type T int
> 2   var x T
>     {
>     3   type T int
>     4   var y T
>     5   _ = x==y
>     }
> }
> 
> The types on lines 2 and 4 refer to different typespecs (lines 1 and 3) so the
> comparison on line 5 is not legal.

This makes sense.


>
https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal_test.go
> File src/pkg/runtime/mfinal_test.go (right):
> 
>
https://codereview.appspot.com/12895043/diff/3001/src/pkg/runtime/mfinal_test...
> src/pkg/runtime/mfinal_test.go:45: {func(x *int) interface{} { return
(*Tint)(x)
> }, func(v Tinter) { finalize((*int)(v.(*Tint))) }},
> On 2013/08/14 09:27:49, dvyukov wrote:
> > Should it work for:
> > v := new(struct{foo int})
> > SetFinalizer(v, func(p *struct{foo int}) {})
> > ?
> > These are identical types, right?
> 
> Yes, that works. I will add a test.

[dvyukov, 2013-08-14 16:35:47]

LGTM with test for unnamed identical types and minor fixes

[rsc, 2013-08-14 16:54:26]

actually, there is already a test for unnamed identical types: *int and
*int.

[dvyukov, 2013-08-14 17:10:33]

On Wed, Aug 14, 2013 at 8:54 PM, Russ Cox <rsc@golang.org> wrote:

> actually, there is already a test for unnamed identical types: *int and
> *int.
>
>
OK

[rsc, 2013-08-14 18:55:51]

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

runtime: make SetFinalizer(x, f) accept any f for which f(x) is valid

Originally the requirement was f(x) where f's argument is
exactly x's type.

CL 11858043 relaxed the requirement in a non-standard
way: f's argument must be exactly x's type or interface{}.

If we're going to relax the requirement, it should be done
in a way consistent with the rest of Go. This CL allows f's
argument to have any type for which x is assignable;
that's the same requirement the compiler would impose
if compiling f(x) directly.

Fixes issue 5368.

R=dvyukov, bradfitz, pieter
CC=golang-dev
https://codereview.appspot.com/12895043