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
Allow declaring weak properties #211
Allow declaring weak properties #211
Changes from 11 commits
884960a
041061d
63ef1c3
e475b93
f2da5ef
e9f2d78
dd83d5d
2d50c47
e606a40
ffa7467
6701965
eaf0586
1b071f5
b3cdb75
1f7f74a
2a514ea
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.
What behavior do we want if a class contains both a user-defined
dealloc
method and properties with dealloc callbacks? The implementation here will call the dealloc callbacks before the user-defineddealloc
method. But in practice I think the opposite order would be more useful - that way the user-defineddealloc
could call methods on objects stored in properties before they are released by the callbacks (which could cause them to be deallocated right away).Implementing this would be a bit more difficult. A user-defined
dealloc
is expected to end with a call to the superclassdealloc
. But if a class has dealloc callbacks, those would need before the superclassdealloc
(but also after the user-defineddealloc
code). There's no way for Rubicon to insert the dealloc callbacks into the user'sdealloc
method, so the user would have to manually call the dealloc callbacks before the super call (via an extra function/method provided by Rubicon).A different solution would be to allow users to define their own dealloc callback that gets called before any dealloc callbacks from properties. Then users could use that callback instead of manually overriding
dealloc
, and let Rubicon generate adealloc
method that calls all callbacks in the right order.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 agree that calling the user's
dealloc
before clearing any instance variables can make life at lot easier for the user. But I don't really like either of the proposed solutions, both look like workarounds that force the user to become aware of the memory management which we are otherwise performing behind the scenes.I can possibly think of two alternative approaches that could provide a better user experience (but would complicate our own implementation):
We document that user should not call the
dealloc
of the super class in their own implementation, as in a proper ARC environment. We then call the following methods in order: First, any user-defined dealloc, second, our cleanup code and third the super class's dealloc. This would mean treating the dealloc definition differently from other method definitions, at least internally. It would however be transparent to the user.We call our own cleanup code after calling the old
delloc
implementation (which includes the user's code and any calls to the super dealloc). I'm not sure if this is possible to do reliably.What do you think, is either of those options worth the effort?
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.
Option 1 would be ideal, because it's simple to use and matches what you do in normal ARC Objective-C code. But implementing this behavior now would silently break existing code that overrides
dealloc
and correctly calls the superclassdealloc
at the end - that would cause the superclassdealloc
to get called twice, which can lead to other objects beingrelease
d too often and other similar problems. That's the main reason why I suggested adding a separate user-definable callback for the same purpose.Rubicon is still before version 1.0, so we could still make breaking changes like this, especially if it improves usability in the long term. But if we do that, we should try to throw errors for code relying on the old behavior, to avoid silent double frees in code that was previously correct.
I honestly don't know if option 2 would be safe or not... There should be no way for the superclass
dealloc
s to corrupt ivars defined by the subclass, but I wouldn't really rely on it. EspeciallyNSObject
's top-leveldealloc
could do things that make the entire object unusable somehow.Another alternative would be to not touch user-defined
dealloc
s at all, and only generate an automaticdealloc
if the user hasn't already overriddendealloc
manually. This would be fully compatible with existing code and should also be simple to implement. The disadvantage is that if the user really needs to add custom code todealloc
, they then have to manually do all the cleanup that the dealloc callbacks would have done automatically.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 do prefer Option 1 from those choices, or not touching use-defined deallocs at all together with a clear documentation on manual cleanup. My only issue with Option 1 is, how can we raise an error if user does call the superclass dealloc? Is there an elegant way of doing so? We are not a compiler after all and don't want to inspect the actual code in the user's dealloc.
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 can confirm that running our cleanup after the superclass dealloc does lead to trouble during the ivar cleanup. In particular,
libobjc.class_getInstanceVariable()
returnsNone
. So this is not viable.I've implemented the first option now: calling the user's dealloc, then our cleanup, then the superclass dealloc. For the time being, there is no special error handling if the user calls the superclass dealloc manually. It will however raise (obscure) errors when we run our own cleanup. Without our own cleanup code (for example when no properties are declared), dealloc is called twice and leads to a segfault. In either case, users will notice that something is wrong without knowing what it might be. Not ideal...
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 best workaround I can come up with, short of actually inspecting the user's dealloc code when creating the class, is to print a warning when
send_super
is called, before either segfaulting or failing to complete the dealloc. What do you think, is this acceptable?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.
Sorry for the late reply - had some uni stuff that I needed to finish in the last few days.
The implementation you wrote looks good IMO. You're right that we can't do much to guard against old code that still calls
send_super
at the end ofdealloc
. The only option I can think of is what you've already suggested - add a special case insidesend_super
that warns wheneversend_super(self, "dealloc", ...)
is called. If we do that, we just need to make sure that the warning doesn't appear when Rubicon itself callssend_super(self, "dealloc", ...)
- probably using an internal keyword to suppress the warning?That way I think we could even "fix" the segfaults. If
send_super(self, "dealloc", ...)
is called without the special internal keyword argument, we can make it show a warning and then return without actually calling the super method. That way, if a user-defineddealloc
callssend_super
, the superdealloc
won't actually be called yet. Then the user-defineddealloc
returns to Rubicon, which runs the dealloc callbacks then makes the realsend_super
call (with the internal keyword argument, so that this time it actually calls the super method).This isn't a very nice solution - but if it works, I would rather have some less nice code in Rubicon that shows a helpful warning about the change, rather than breaking existing code so that it causes unexplained segfaults.
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.
That said, it looks like our existing
add_method
implementation doesn't check the return value ofclass_addMethod
, so the method would silently do nothing if a conflicting method already exists. So if we keepadd_method
we should also fix it to raise an error ifclass_addMethod
returns false.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.
When is this cast required? Isn't it sufficient to to return a
objc_id
which later gets converted to the correctObjCInstance
?If it is required, do we need to add a similar cast before returning the value in the weak case?
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.
You're right that
libobjc.object_getIvar
already returns anobjc_id
. The most common case is thatvartype
is exactlyobjc_id
, in which case the cast indeed does nothing. I think it's only needed for the less common case wherevartype
isClass
(which we have declared as a subclass ofobjc_id
) so that the return value is cast toClass
instead of a plainobjc_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.
Ah, ok! So we should perform a similar cast when returning a value in the weak case as well.