Remove reliance on shared_ptr for ASDisplayNodeLayouts #1131
Conversation
Adlai-Holler
requested review from
maicki,
garrettmoon,
appleguy and
nguyenhuy
Source/ASDisplayNode+Layout.mm
Outdated
| constrainedSize, | ||
| constrainedSize.max, | ||
| newLayoutVersion); | ||
| var pendingLayout = ASDisplayNodeLayout(newLayout, |
There was a problem hiding this comment.
Do we change the pendingLayout somewhere to change it to var?
There was a problem hiding this comment.
You're right – in a previous version, setCalculatedLayout mutated the input (to apply the unflattenedLayout thing) but I changed it to copy the input and never committed reverting this to let.
| @@ -43,10 +43,16 @@ struct ASDisplayNodeLayout { | |||
| /** | |||
| * Returns whether this is valid for a given version | |||
| */ | |||
| BOOL isValid(NSUInteger version); | |||
| BOOL isValid(NSUInteger versionArg) { | |||
There was a problem hiding this comment.
You think it's worth it to inline?
There was a problem hiding this comment.
I figure there's no real downside. What do you think?
Source/Private/ASLayoutTransition.h
Outdated
| @@ -52,12 +50,12 @@ AS_SUBCLASSING_RESTRICTED | |||
| /** | |||
| * Previous layout to transition from | |||
| */ | |||
| @property (nonatomic, readonly) std::shared_ptr<ASDisplayNodeLayout> previousLayout; | |||
| @property (nonatomic, readonly) const ASDisplayNodeLayout &previousLayout; | |||
There was a problem hiding this comment.
Are we sure the ref lifes for the full lifetime of the transition?
There was a problem hiding this comment.
It's an ivar on the transition object (it's copied in -init) so it'll live as long as the transition lives.
There was a problem hiding this comment.
There is a small risk here, which is you read this ref and store it/don't copy it, and then you release the transition while you're holding on to the ref. Our current usage is OK. We could actually change this to be a pointer, and use NS_RETURNS_INNER_POINTER to prevent that … I might try it.
EDIT: Apparently NS_RETURNS_INNER_POINTER works with C++ refs, so I added it.
| @@ -43,10 +43,16 @@ struct ASDisplayNodeLayout { | |||
| /** | |||
| * Returns whether this is valid for a given version | |||
| */ | |||
| BOOL isValid(NSUInteger version); | |||
| BOOL isValid(NSUInteger versionArg) { | |||
…1131) * Remove reliance on shared_ptr for ASDisplayNodeLayouts * Fix up * Fix in yoga * Back to let * Returns inner pointer * Trivial change to kick the CI
No description provided.