Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Add ObjC Block Support #261
base: main
Are you sure you want to change the base?
Add ObjC Block Support #261
Changes from 1 commit
25c5827
e7d3e21
c4394b5
8b1d9fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
//
style comments for consistency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just do this check first and then call the encodeFunc instead of copying the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be exported as it is a method on an unexpected type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: these parenthesis aren't necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for keeping this as an opaque pointer instead of just *blockLayout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 asuintptr
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does have methods (most importantly
Copy()
andRelease()
for lifetime management), but taking your thoughts into account, this could be refactored to behave likeProtocol
, which is used as a struct pointer, even though it, too, is actually a first-class object?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean there are no objc methods that users are trying to call.
Yes that's what I'm thinking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this be exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Perhaps then, just replace this method with
Implementation()
(to treat it like a read-only property) that returns auintptr
, and allow the user to callRegisterFunc()
at their own discretion (keeping in mind, the lifetime caveats, of course?), with the option to use just use anInvoke()
orBlockInvoke[]()
that have already been implemented with lifetime-safety in mind?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to write this as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIll fix. Note that the
cfn :=
clause would need to be pulled out of theif
block to actually work, but simple enough.