Add ObjC Block Support #261
Conversation
Adds new type, objc.Block, which is an objc.ID referencing an Objective-C "block" function pointer. Adds methods to create a Block from a Go function value, get a Go function value from a block, directly invoke a block function, and handle Objective-C memory management (e.g. Copy/Release). Mitigates pressure on purego Callback limit by relying on the fact the first argument passed to a block implementation is the block itself. This allows for a single callback to handle every block instance that has the same signature, by way of keeping an association between the Go func value and the block instance it was used to create. Was refactored from code in a different personal project to better fit the purego convetions and architecture. Directly addresses (closed) purego issue: ebitengine#129
|
Please address on test failures |
| // otherwise: create a layout, and populate it with the default templates | ||
| layout := b.layoutTemplate | ||
| layout.Descriptor = &blockDescriptor{} | ||
| (*layout.Descriptor) = b.descriptorTemplate |
There was a problem hiding this comment.
nitpick: these parenthesis aren't necessary
| // also, it creates a new copy of the block which must be freed independently, | ||
| // which would have made this implementation more complicated than necessary. | ||
| // we know a block ID is actually a pointer to a blockLayout struct, so we'll take advantage of that. | ||
| if b != 0 { |
There was a problem hiding this comment.
I think it's better to write this as:
if b == 0 {
return
}
if cfn := (*(**blockLayout)(unsafe.Pointer(&b))).Invoke; cfn == 0 {
return
}
purego.RegisterFunc(fptr, cfn)
There was a problem hiding this comment.
WIll fix. Note that the cfn := clause would need to be pulled out of the if block to actually work, but simple enough.
| var blocks *blockCache | ||
|
|
||
| // Block is an opaque pointer to an Objective-C object containing a function with its associated closure. | ||
| type Block ID |
There was a problem hiding this comment.
What's the reason for keeping this as an opaque pointer instead of just *blockLayout?
There was a problem hiding this comment.
A "Block" in Objective-C is first-class object (e.g. can be cast to an ID and sent messages like any other object.) The problem is, ID in purego's objc implementation has a base of uintptr, so defining a block as a pointer type would complicate down-casting. I believe the choice to define ID as uintptr instead of *struct{...} (which would better align to what it actually is in Objective-C) was to allow receiver methods to be defined (since go does not allow receiver methods on pointer type aliases), but that's just speculation/intuition.
There was a problem hiding this comment.
Are people going to actually cast Block to ID? Block doesn't have any methods does it?
Of course if a class extended Block then you'd probably want ID but then are you using it as a Block still?
Perhaps a Block.ID() method and BlockFromID() function that does the unsafe cast? Perhaps that can be added later as I'm not confident people need to use a Block as an ID
There was a problem hiding this comment.
It does have methods (most importantly Copy() and Release() for lifetime management), but taking your thoughts into account, this could be refactored to behave like Protocol, which is used as a struct pointer, even though it, too, is actually a first-class object?
There was a problem hiding this comment.
I mean there are no objc methods that users are trying to call.
this could be refactored to behave like Protocol
Yes that's what I'm thinking
| func(block objc.Block, line objc.ID, stop *bool) { | ||
| count++ | ||
| fmt.Printf("LINE %d: %s\n", count, objc.Send[string](line, objc.RegisterName("UTF8String"))) | ||
| (*stop) = (count == 3) |
There was a problem hiding this comment.
nitpick: neither of these parentheses are necessary
| ) | ||
|
|
||
| func ExampleNewBlock() { | ||
| _, err := purego.Dlopen("/System/Library/Frameworks/Foundation.framework/Foundation", purego.RTLD_GLOBAL) |
There was a problem hiding this comment.
Although it isn't necessary it's good to add purego.RTLD_NOW| as that's the proper way to call dlopen
There was a problem hiding this comment.
Okay, will fix. I actually copied the syntax from the existing load of libobjc.dylib.
There was a problem hiding this comment.
Please fix it there too
| // GetImplementation populates a function pointer with the implementation of a Block. | ||
| // Function will panic if the Block is not kept alive while it is in use | ||
| // (possibly by using Block.Copy()). | ||
| func (b Block) GetImplementation(fptr any) { |
There was a problem hiding this comment.
Why should this be exported?
There was a problem hiding this comment.
I wasn't actually happy with it either. It was meant to provide provide synergy with purego's RegisterFunc in the block case, but has potential for misuse because of blocks having managed lifetimes.
There was a problem hiding this comment.
If someone wants to call a Block directly can't they just call RegisterFunc? It's just a C function with a objc.Block as the first argument current?
There was a problem hiding this comment.
Indeed. Perhaps then, just replace this method with Implementation() (to treat it like a read-only property) that returns a uintptr, and allow the user to call RegisterFunc() at their own discretion (keeping in mind, the lifetime caveats, of course?), with the option to use just use an Invoke() or BlockInvoke[]() that have already been implemented with lifetime-safety in mind?
There was a problem hiding this comment.
Do you have a use case where grabbing the raw uintptr to call directly is helpful? I want to limit the exposed API to only the necessary parts
|
|
||
| // GetLayout retrieves a blockLayout VALUE constructed with the supplied function type | ||
| // It will panic if the type is not a valid block function. | ||
| func (b *blockCache) GetLayout(typ reflect.Type) blockLayout { |
There was a problem hiding this comment.
This shouldn't be exported as it is a method on an unexpected type
| // encode returns a blocks type as if it was given to @encode(typ) | ||
| func (*blockCache) encode(typ reflect.Type) *uint8 { | ||
| // this algorithm was copied from encodeFunc, | ||
| // but altered to panic on error, and to only accep a block-type signature. |
There was a problem hiding this comment.
I would just do this check first and then call the encodeFunc instead of copying the code.
if type.Kind() == reflect.Func && ((typ.NumIn() == 0) || (typ.In(0) != reflect.TypeOf(Block(0)))) {
panic(fmt.Sprintf("objc: A Block implementation must take a Block as its first argument; got %v", typ.String()))
}
encoding, err := encodeFunc(fn)
if err != nil {
panic(fmt.Sprintf("objc: failed to encode Block %s", err))
}
``
There was a problem hiding this comment.
The problem is, encodeFunc (as it stands) has a check that enforces an Objective-C method signature, which is not the same as a block signature. Maybe worth splitting up that functon from it's check first, and having both implementations share?
There was a problem hiding this comment.
If you think you can find a good solution go for it otherwise it might be ok to just leave it how you have it now
| The function closures themselves are kept alive by caching them internally until the Objective-C runtime indicates that | ||
| they can be released (presumably when the reference count reaches zero.) This approach is used instead of appending the function | ||
| object to the block allocation, where it is out of the visible domain of Go's GC. | ||
| */ |
There was a problem hiding this comment.
Use // style comments for consistency.
|
This is for passing struct arguments that are not pointers? |
|
No this is for objc callbacks which are called Blocks |
What issue is this addressing?
Updates #129
What type of issue is this addressing?
feature
What this PR does
Adds new type, objc.Block, which is an objc.ID referencing an Objective-C "block" function pointer.
Adds methods to create a Block from a Go function value, get a Go function value from a block, directly invoke a block function, and handle Objective-C memory management (e.g. Copy/Release).
Mitigates pressure on purego Callback limit by relying on the fact the first argument passed to a block implementation is the block itself. This allows for a single callback to handle every block instance that has the same signature, by way of keeping an association between the Go func value and the block instance it was used to create.