Fixed infinite recursion in NSCharacterSet.isEqual #1092
Conversation
| @@ -50,13 +50,18 @@ open class NSCharacterSet : NSObject, NSCopying, NSMutableCopying, NSCoding { | |||
| } | |||
|
|
|||
| open override func isEqual(_ value: Any?) -> Bool { | |||
| guard let runtimeClass = _CFRuntimeGetClassWithTypeID(CFCharacterSetGetTypeID()) else { | |||
There was a problem hiding this comment.
this is rather sub-optimal of a way to approach the type testing
There was a problem hiding this comment.
@phausler Could you please explain a bit more? Isn't this what CFEqual does under the hood?
I'm open for suggestions.
There was a problem hiding this comment.
Yea, but the problem is that it is reaching into CF internals pretty heavily - I have a patch in the works that will resolve this (and passes your unit test) without needing to reach into CF guts so much. This is going the opposite direction of the factory version of NSCharacterSet (which generalizes this method without needing to reach into CF internals)
There was a problem hiding this comment.
@phausler I assumed it was good enough because I didn't need to expose anything from CF.
I also noticed you were working around this area and expected your changed would address this as well, but was not sure if and when.
When do you expect to have those in?
And do you suggest dropping this PR?
There was a problem hiding this comment.
@phausler oh, and by the way, I'm not sure what to think of:
https://github.com/apple/swift-corelibs-foundation/pull/1092/files#diff-c6672fda2d6b50ba301f8b945cacbd8aR298
Any thoughts?
There was a problem hiding this comment.
In this branch I am working in I have those tests enabled as well (and they are passing). I have a few more things to wrap up before pushing that as an addendum to the factory_generalizations. (it isn't ready for pushing yet since I am working on getting a few more goodies published as well)
|
hmm it looks like my pr is going to take a bit to get all the new bits approved - so lets go ahead and merge this for now since it is blocking testing of Codable to be ported. My new stuff will refactor the implementation but I will make certain your tests are included. |
|
@swift-ci please test and merge |
CFEqualforCFCharacterSetwould callNSCharacteSet.isEqualwhich would again callCFEqualand so on.