-
Notifications
You must be signed in to change notification settings - Fork 808
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
Implement NSIndexSet enumeration and *PassingTest apis #2268
Conversation
include/Foundation/NSIndexSet.h
Outdated
- (NSIndexSet*)indexesWithOptions:(NSEnumerationOptions)opts passingTest:(BOOL (^)(NSUInteger, BOOL*))predicate; | ||
- (NSUInteger)indexInRange:(NSRange)range options:(NSEnumerationOptions)opts passingTest:(BOOL (^)(NSUInteger, BOOL*))predicate; | ||
- (NSIndexSet*)indexesInRange:(NSRange)range options:(NSEnumerationOptions)opts passingTest:(BOOL (^)(NSUInteger, BOOL*))predicate; | ||
- (void)enumerateRangesInRange:(NSRange)range options:(NSEnumerationOptions)opts usingBlock:(void (^)(NSRange, BOOL*))block; |
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.
opts [](start = 76, length = 4)
nit: Update var names to reflect changes in NSIndexSet.mm
Frameworks/Foundation/NSIndexSet.mm
Outdated
UNIMPLEMENTED(); | ||
return StubReturn(); | ||
- (NSUInteger)indexInRange:(NSRange)range options:(NSEnumerationOptions)options passingTest:(BOOL (^)(NSUInteger, BOOL*))predicate { | ||
NSIndexSet* set = [self indexesInRange:range options:options passingTest:predicate]; |
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.
*stop
optimization?
Frameworks/Foundation/NSIndexSet.mm
Outdated
- (void)enumerateRangesInRange:(NSRange)range options:(NSEnumerationOptions)opts usingBlock:(void (^)(NSRange, BOOL*))block { | ||
UNIMPLEMENTED(); | ||
- (void)enumerateRangesUsingBlock:(void (^)(NSRange, BOOL*))block { | ||
[self enumerateRangesWithOptions:0 usingBlock:block]; |
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.
please always call the most-specific version in these shim implementations; you'll avoid an entire level of dynamic dispatch and a stack frame.
} | ||
|
||
if (indexRange) { | ||
*indexRange = { j, range.location + range.length - j }; |
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.
huh; indexRange is in/out?
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.
yup 🇫🇰
for (NSUInteger i = start; i != end && stop == NO; i += step) { | ||
NSRange intersection = NSIntersectionRange(range, ranges[i - 1]); | ||
if (intersection.length != 0) { | ||
NSUInteger rangeStart = reverse ? intersection.location + intersection.length : intersection.location + 1; |
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, this DOES seem like it should be able to be lifted out/commonized.
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.
IKR. Unfortunately, in the concurrent case we'd be doing two levels of dispatch_group_async which seems sillier than very similar but not identical code
Frameworks/Foundation/NSIndexSet.mm
Outdated
} | ||
|
||
return ret; | ||
}]; | ||
return (options & NSEnumerationReverse) ? [set lastIndex] : [set firstIndex]; |
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.
since set will contain only one index, this is no longer necessary
nit: because this is a set, the set will set set |
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.
(set)
Fixes #2076
Fixes #2077