Copy yogaChildren in accessor method. Avoid using accessor method internally #1283
Conversation
| @@ -58,7 +58,7 @@ - (void)setYogaChildren:(NSArray *)yogaChildren | |||
|
|
|||
| - (NSArray *)yogaChildren | |||
| { | |||
| return _yogaChildren; | |||
| return [_yogaChildren copy] ?: @[]; | |||
There was a problem hiding this comment.
We already lock on the setter, why not go ahead and lock here and declare the property atomic?
There was a problem hiding this comment.
Because this diff was copied from an 11th-hour internal hotfix! But you're right, we should do it now. @maicki mind making the change and landing it with this?
There was a problem hiding this comment.
I knew I'd been here before...
Source/ASDisplayNode+Beta.h
Outdated
| @@ -189,7 +189,7 @@ AS_EXTERN void ASDisplayNodePerformBlockOnEveryYogaChild(ASDisplayNode * _Nullab | |||
| @interface ASDisplayNode (Yoga) | |||
|
|
|||
| // TODO: Make this and yogaCalculatedLayout atomic (lock). | |||
| @property (nullable, nonatomic) NSArray *yogaChildren; | |||
| @property (nonatomic, copy) NSArray *yogaChildren; | |||
There was a problem hiding this comment.
I don't think there is a keyword to specify copy-on-read, this one is supposed to mean copy-on-set
There was a problem hiding this comment.
You mean the keyword means both, or this class is somehow both? Oh the second, yes, I see we do basically copy after all.
There was a problem hiding this comment.
Here's the official doc, it's pretty unclear because they assume your ivar is immutable. https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html
If your ivar is mutable and the property is atomic, there's no other choice than copying on read. Our property isn't atomic, but it should be and we badly assume that it is.
There was a problem hiding this comment.
The official doc doesn't seem unclear at all: "Copy Properties Maintain Their Own Copies"
But in our case, we are copying in our setter too (which I missed at first), so I agree copy is a correct attribute.
There was a problem hiding this comment.
The docs are unclear unless you read them to mean that we are not allowed to use a mutable ivar i.e. we have to "maintain our own copy" as an immutable variable at all times. If so, then screw the docs.
There was a problem hiding this comment.
The documentation happens to use immutable copies in the examples, but I don't see anything stating or implying that's required (a mutable backing var, as we can see, becomes more complicated, which is why it wouldn't have been great for example code anyway). So no screwing required!
There was a problem hiding this comment.
Because it's assumed by the example code, there's no guidance on the semantics of getters that are backed by mutable ivars. That's the problem with using it as an example!
maicki
force-pushed
the
MSCopyYogaChildren
branch
from
11f8a13
to
bb0231f
Compare
|
@Adlai-Holler @wiseoldduck I don't remember where we land on, but could you give that another look please, I updated the |
maicki
force-pushed
the
MSCopyYogaChildren
branch
from
c9a0ed3
to
732460e
Compare
No description provided.