CharacterSet : Codable implementation

[bubski]

Continuation of #1108
This PR mirrors some changes from swiftlang/swift#9715

---

Added conformances + unit tests for

• CharacterSet
  CharacterSet : Equatable was not working correctly in some cases and was fixed to make Codable unit tests for CharacterSet work.

Not included in this PR, because of some problems that need to be solved

• Calendar
• DateComponents
• Measurement
• TimeZone

Already implemented

• PersonNameComponents
• UUID
• URL
• NSRange
• Locale
• IndexSet
• IndexPath
• AffineTransform
• Decimal
• DateInterval
• CGFloat (unit tested indirectly)

[itaiferber]

The change looks reasonable overall, but I think we can simplify the tests a bit.

[itaiferber]

What changed here? Is there an effective difference? A comment would be helpful.

[bubski]

I'll add a comment.

The problem was rhs._bridgeToObjectiveC() would always return an invalid (empty) CharacterSet.
._mapUnmanaged behaved correctly.

Now that I think about it, it can actually be changed to

return lhs._mapUnmanaged { $0.isEqual(rhs) }

PS
I actually fixed ._bridgeToObjectiveC() in this PR too, so
lhs._wrapped.isEqual(rhs._bridgeToObjectiveC()) would start working correctly,
but I think lhs._mapUnmanaged { $0.isEqual(rhs) } follows better the pattern seen in other methods of CharacterSet, like

    public func isSuperset(of other: CharacterSet) -> Bool {
        return _mapUnmanaged { $0.isSuperset(of: other) }
    }

Does that make sense @itaiferber ?

[itaiferber]

Yep, reasonable, and makes sense. Comments both in here and in _bridgeToObjectiveC() explaining what makes this work (vs. doesn't) would be nice. Thank you! 🙂

[itaiferber]

If keeping the Box is helpful/necessary, then the following comment applies: All of these new asserts seem a bit out of place. Is it not sufficient to make the Box Equatable and perform these checks in ==? (The unit tests can then continue using XCTAssertEqual.)

[bubski]

@itaiferber These are very good suggestions, thanks. I'll revisit this. And update this PR.

I wanted to make sure various different equality check combinations are covered, because I was running into inconsistent results in unexpected scenarios.

[bubski]

@itaiferber I've done the tests refactor. Much cleaner 👍
Please let me know if it looks ok to you now.

[itaiferber]

Is this Box necessary? This doesn't seem to be significantly less work than just replicating some tests:

for (lhs, rhs) in pairs {
    XCTAssertEqual(NSCharacterSet(charactersIn: lhs), NSCharacterSet(charactersIn: rhs))
    XCTAssertEqual(CharacterSet(charactersIn: lhs), CharacterSet(charactersIn: rhs))
}

[bubski]

@phausler I'd like to run this against you and ask for advice.

I'm not sure if this solutionis ok.

Perhaps it makes more sense to keep return _wrapped here, and add

override var _cfObject: CFType {
    return _mapUnmanaged { unsafeBitCast($0, to: CFType.self) }
}

to _SwiftNSCharacterSet

The problem I'm trying to solve is that we return _wrapped here, which is _SwiftNSCharacterSet.
Somewhere later, we access ._cfObject in this object, which does unsafeBitCast(self, to: CFType.self).
But this doesn't work correctly, because _SwiftNSCharacterSet has different structure than CFCharacterSet.

[ianpartridge]

@parkera ^^^

[itaiferber]

@bubski The new solution sounds reasonable to me. Have you tested locally to confirm the expected behavior?

[bubski]

@itaiferber yes, I have 👌 seems to be working ok.

But I'm not that comfortable with the way copy-on-write is set up yet, so would like someone to double-check this please.

[itaiferber]

These changes look good to me!

[itaiferber]

@swift-ci Please test

[ianpartridge]

@swift-ci please test and merge