Skip to content
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

Reimplement ASRectTable using unordered_map to avoid obscure NSMapTable exception. #719

Merged
merged 3 commits into from
Dec 22, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## master
* Add your own contributions to the next release on the line below this with your name.
- [ASRectMap] Replace implementation of ASRectTable with a simpler one based on unordered_map.[Scott Goodson](https://github.com/appleguy) [#719](https://github.com/TextureGroup/Texture/pull/719)
- [ASNetworkImageNode] Deprecates .URLs in favor of .URL [Garrett Moon](https://github.com/garrettmoon) [#699](https://github.com/TextureGroup/Texture/pull/699)
- [iOS11] Update project settings and fix errors [Eke](https://github.com/Eke) [#676](https://github.com/TextureGroup/Texture/pull/676)
- [ASCollectionView] Improve performance and behavior of rotation / bounds changes. [Scott Goodson](https://github.com/appleguy) [#431](https://github.com/TextureGroup/Texture/pull/431)
Expand Down
10 changes: 5 additions & 5 deletions Source/Layout/ASLayout.mm
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#import <AsyncDisplayKit/ASEqualityHelpers.h>
#import <AsyncDisplayKit/ASInternalHelpers.h>
#import <AsyncDisplayKit/ASObjectDescriptionHelpers.h>
#import <AsyncDisplayKit/ASRectTable.h>
#import <AsyncDisplayKit/ASRectMap.h>

CGPoint const ASPointNull = {NAN, NAN};

Expand Down Expand Up @@ -86,7 +86,7 @@ @interface ASLayout () <ASDescriptionProvider>
*/
@property (nonatomic, strong) NSMutableArray<id<ASLayoutElement>> *sublayoutLayoutElements;

@property (nonatomic, strong, readonly) ASRectTable<id<ASLayoutElement>, id> *elementToRectTable;
@property (nonatomic, strong, readonly) ASRectMap *elementToRectMap;

@end

Expand Down Expand Up @@ -143,9 +143,9 @@ - (instancetype)initWithLayoutElement:(id<ASLayoutElement>)layoutElement
_sublayouts = sublayouts != nil ? [sublayouts copy] : @[];

if (_sublayouts.count > 0) {
_elementToRectTable = [ASRectTable rectTableForWeakObjectPointers];
_elementToRectMap = [ASRectMap rectMapForWeakObjectPointers];
for (ASLayout *layout in sublayouts) {
[_elementToRectTable setRect:layout.frame forKey:layout.layoutElement];
[_elementToRectMap setRect:layout.frame forKey:layout.layoutElement];
}
}

Expand Down Expand Up @@ -303,7 +303,7 @@ - (ASLayoutElementType)type

- (CGRect)frameForElement:(id<ASLayoutElement>)layoutElement
{
return _elementToRectTable ? [_elementToRectTable rectForKey:layoutElement] : CGRectNull;
return _elementToRectMap ? [_elementToRectMap rectForKey:layoutElement] : CGRectNull;
}

- (CGRect)frame
Expand Down
34 changes: 9 additions & 25 deletions Source/Private/ASRectTable.h → Source/Private/ASRectMap.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// ASRectTable.h
// ASRectMap.h
// Texture
//
// Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
Expand All @@ -17,56 +17,40 @@

#import <Foundation/Foundation.h>
#import <CoreGraphics/CGGeometry.h>
#import <AsyncDisplayKit/ASBaseDefines.h>

NS_ASSUME_NONNULL_BEGIN

/**
* An alias for an NSMapTable created to store rects.
*
* You should not call -objectForKey:, -setObject:forKey:, or -allObjects
* on these objects.
*/
typedef NSMapTable ASRectTable;

/**
* A category for creating & using map tables meant for storing CGRects.
*
* This category is private, so name collisions are not worth worrying about.
*/
@interface NSMapTable<KeyType, id> (ASRectTableMethods)

/**
* Creates a new rect table with (NSMapTableStrongMemory | NSMapTableObjectPointerPersonality) for keys.
* A category for indexing weak pointers to CGRects. Similar to ASIntegerMap.
*/
+ (ASRectTable *)rectTableForStrongObjectPointers;
@interface ASRectMap : NSObject

/**
* Creates a new rect table with (NSMapTableWeakMemory | NSMapTableObjectPointerPersonality) for keys.
* Creates a new rect map. The keys are never retained.
*/
+ (ASRectTable *)rectTableForWeakObjectPointers;
+ (ASRectMap *)rectMapForWeakObjectPointers;

/**
* Retrieves the rect for a given key, or CGRectNull if the key is not found.
*
* @param key An object to lookup the rect for.
*/
- (CGRect)rectForKey:(KeyType)key;
- (CGRect)rectForKey:(id)key;

/**
* Sets the given rect for the associated key.
* Sets the given rect for the associated key. Key *will not be retained!*
*
* @param rect The rect to store as value.
* @param key The key to use for the rect.
*/
- (void)setRect:(CGRect)rect forKey:(KeyType)key;
- (void)setRect:(CGRect)rect forKey:(id)key;

/**
* Removes the rect for the given key, if one exists.
*
* @param key The key to remove.
*/
- (void)removeRectForKey:(KeyType)key;
- (void)removeRectForKey:(id)key;

@end

Expand Down
83 changes: 83 additions & 0 deletions Source/Private/ASRectMap.mm
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//
// ASRectMap.mm
// Texture
//
// Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
// This source code is licensed under the BSD-style license found in the
// LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
// grant of patent rights can be found in the PATENTS file in the same directory.
//
// Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
// Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this file license per Danger's comment.

//
//  ASRectMap.mm
//  Texture
//
//  Copyright (c) 2017-present, Pinterest, Inc.  All rights reserved.
//  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//


#import "ASRectMap.h"
#import "ASObjectDescriptionHelpers.h"
#import <UIKit/UIGeometry.h>
#import <unordered_map>

@implementation ASRectMap {
std::unordered_map<void *, CGRect> _map;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may should add a comment that we use a void * key here as arc would not be able to determine the ownership qualification.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we not use __unsafe_unretained id as the key type here?

Copy link
Contributor

@maicki maicki Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't think we can use __unsafe_unretained id as key. The unordered_map template cannot figure out the hash for the __unsafe_unretained id key. We would have to write our own key like:

struct ASRectMapMapKey {
    __unsafe_unretained id obj;
    
    bool operator==(const ASRectMapMapKey &other) const
    {
        return [obj isEqual:other.obj];
    }
};

namespace std {
    template<> struct hash<ASRectMapMapKey>
    {
        size_t operator()(const ASRectMapMapKey &k) const
        {
            return [k.obj hash];
        }
    };
}

And we could use it via:

std::unordered_map<ASRectMapMapKey, CGRect> map;
map.insert({{@"key"}, CGRectMake(0, 0, 100, 100)});
NSLog(@"%@", NSStringFromRect(map.at({@"key"})));

}

+ (ASRectMap *)rectMapForWeakObjectPointers
{
return [[self alloc] init];
}

- (CGRect)rectForKey:(id)key
{
auto result = _map.find((__bridge void *)key);
if (result != _map.end()) {
// result->first is the key; result->second is the value, a CGRect.
return result->second;
} else {
return CGRectNull;
}
}

- (void)setRect:(CGRect)rect forKey:(id)key
{
if (key) {
_map[(__bridge void *)key] = rect;
}
}

- (void)removeRectForKey:(id)key
{
if (key) {
_map.erase((__bridge void *)key);
}
}

- (id)copyWithZone:(NSZone *)zone
{
ASRectMap *copy = [ASRectMap rectMapForWeakObjectPointers];
copy->_map = _map;
return copy;
}

- (NSMutableArray<NSDictionary *> *)propertiesForDescription
{
NSMutableArray *result = [NSMutableArray array];

// { ptr1->rect1 ptr2->rect2 ptr3->rect3 }
NSMutableString *str = [NSMutableString string];
for (auto it = _map.begin(); it != _map.end(); it++) {
Copy link
Contributor

@maicki maicki Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use of a bit of a smaller code:

for ( auto const& entry : _map ) {
    [str appendFormat:@" %@->%@", entry->first, NSStringFromCGRect(entry->second)];
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method dangerous, now that the keys are stored unretained? Could we be calling -description on a garbage pointer? We could fall back to %p.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @Adlai-Holler. Let's pay extra attention here.

[str appendFormat:@" %@->%@", it->first, NSStringFromCGRect(it->second)];
}
[result addObject:@{ @"ASRectMap": str }];

return result;
Copy link
Member

@Adlai-Holler Adlai-Holler Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class name will already be in the description by default, so I think what we want here is @{ (id)kCFNull : str } which will just dump the list of pointers inside e.g.: <ASRectMap: 0xFFFFFFFF; "a->b, c->d">

Edit: NM I see you used MakeWithoutObject so we exclude the pointer from the description a la other collection classes 👍

}

- (NSString *)description
{
return ASObjectDescriptionMakeWithoutObject([self propertiesForDescription]);
}

@end
80 changes: 0 additions & 80 deletions Source/Private/ASRectTable.m

This file was deleted.

14 changes: 7 additions & 7 deletions Tests/ASRectTableTests.m → Tests/ASRectMapTests.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// ASRectTableTests.m
// ASRectMapTests.m
// Texture
//
// Created by Adlai Holler on 2/24/17.
Expand All @@ -8,17 +8,17 @@

#import <XCTest/XCTest.h>

#import "ASRectTable.h"
#import "ASRectMap.h"
#import "ASXCTExtensions.h"

@interface ASRectTableTests : XCTestCase
@interface ASRectMapTests : XCTestCase
@end

@implementation ASRectTableTests
@implementation ASRectMapTests

- (void)testThatItStoresRects
{
ASRectTable *table = [ASRectTable rectTableForWeakObjectPointers];
ASRectMap *table = [ASRectMap rectMapForWeakObjectPointers];
NSObject *key0 = [[NSObject alloc] init];
NSObject *key1 = [[NSObject alloc] init];
ASXCTAssertEqualRects([table rectForKey:key0], CGRectNull);
Expand All @@ -35,13 +35,13 @@ - (void)testThatItStoresRects

- (void)testCopying
{
ASRectTable *table = [ASRectTable rectTableForWeakObjectPointers];
ASRectMap *table = [ASRectMap rectMapForWeakObjectPointers];
NSObject *key = [[NSObject alloc] init];
ASXCTAssertEqualRects([table rectForKey:key], CGRectNull);
CGRect rect0 = CGRectMake(0, 0, 100, 100);
CGRect rect1 = CGRectMake(0, 0, 50, 50);
[table setRect:rect0 forKey:key];
ASRectTable *copy = [table copy];
ASRectMap *copy = [table copy];
[copy setRect:rect1 forKey:key];

ASXCTAssertEqualRects([table rectForKey:key], rect0);
Expand Down