Skip to content

runtime: allow SetFinalizer with a func(interface{}) #5368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
PieterD opened this issue Apr 29, 2013 · 14 comments
Closed

runtime: allow SetFinalizer with a func(interface{}) #5368

PieterD opened this issue Apr 29, 2013 · 14 comments
Labels
FrozenDueToAge
Milestone
Go1.2

Comments

@PieterD
Copy link
Contributor

PieterD commented Apr 29, 2013 

runtime.SetFinalizer() only accepts arguments like so:
SetFinalizer(*T, func(*T))

This makes it hard to use a general finalizer for multiple types, to set finalizers on
interfaces, and to set finalizers on objects created using reflect.New().

The proposal is to support runtime.SetFinalizer with a function that takes a single
argument of type interface{}.
@bradfitz
Copy link
Contributor

Comment 1:

Can't you use reflect.MakeFunc?

@ianlancetaylor
Copy link
Member

Comment 2:

Explained on mailing list: doesn't work today because there is no FuncOf.
In any case MakeFunc is kind of complex for a relatively simple operation.

Labels changed: added packagechange.

@PieterD
Copy link
Contributor Author

PieterD commented Apr 29, 2013

Comment 3:

What would be required to implement this? Obviously the extra type checks need to be
added in pkg/runtime/malloc.goc.
What about calling the finalizers, in pkg/runtime/mgc0.c runfinq()?

@ianlancetaylor
Copy link
Member

Comment 4:

In runfinq, if the function took an argument of type interface{}, f->arg would have to
be converted to type interface{}.  Probably we would want SetFinalizer to record the
pointer-to-argument type along with the finalizer.  Then runfinq could use that type,
plus f->arg, to build the interface value to pass to reflect·call.

@PieterD
Copy link
Contributor Author

PieterD commented May 1, 2013

Comment 5:

I've implemented this (hopefully correctly), added some tests, and updated the docs.
Can I send the code for review, or do you want me to sit on it until after Go1.1?

@ianlancetaylor
Copy link
Member

Comment 6:

After Go 1.1 please.
Thanks for working on it.

Status changed to Started.

@rsc
Copy link
Contributor

rsc commented Jul 25, 2013

Comment 7:

Is there a CL pending for this?

Labels changed: added priority-later, go1.2maybe, removed priority-triage.

@PieterD
Copy link
Contributor Author

PieterD commented Jul 25, 2013

Comment 8:

I did make one before Go 1 was released, and kind of forgot. I'll see about updating and
sending it today.

@PieterD
Copy link
Contributor Author

PieterD commented Jul 25, 2013

Comment 9:

CL sent. https://golang.org/cl/11858043/#ps2001
I've never delved this deep into the Runtime before. Good times were had by all.

@PieterD
Copy link
Contributor Author

PieterD commented Jul 29, 2013

Comment 10:

Dmitri LGTM'd the CL a few days ago. What's the procedure? Do I keep the change
up-to-date with recent changes every once in a while until it gets committed?

@dvyukov
Copy link
Member

dvyukov commented Jul 29, 2013

Comment 11:

Is it decided that it should go it? I mean that it extends public API, so we must be
sure.
Ian seem to vote for this, right?
I will land it today. Does it apply to tip now?

@PieterD
Copy link
Contributor Author

PieterD commented Jul 29, 2013

Comment 13:

Ian seemed to agree back in April when I pushed it. I don't know what Russ thinks.
Anyway, yeah it should be on the tip, I remember having to re-patch the code because I
switched.
Thank you for your time, Dmitry.

@dvyukov
Copy link
Member

dvyukov commented Jul 29, 2013

Comment 14:

This issue was closed by revision 6350e45.

Status changed to Fixed.

@rsc
Copy link
Contributor

rsc commented Aug 14, 2013

Comment 15:

This issue was closed by revision 5822e78.

@rsc rsc added this to the Go1.2 milestone Apr 14, 2015
@rsc rsc removed the go1.2maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.2
Development

No branches or pull requests
6 participants
@bradfitz @rsc @PieterD @dvyukov @ianlancetaylor @gopherbot