Skip to content

Use CFNumberCreate() as factory initializer on NSNumber #1093

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jul 3, 2017
Merged

Use CFNumberCreate() as factory initializer on NSNumber #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0a7ddcb
Add `TestNSNumber.test_objCType()` reproducing SR-4907
norio-nomura Jun 1, 2017
67c7981
Add missing entry to `TestNumber.allTests`
norio-nomura Jun 3, 2017
00737ae
Change `NSNumber.init(value: Bool)` to returning `__NSCFBoolean` by u…
norio-nomura Jun 2, 2017
3cbad1d
Change `JSONSerialization` to use `_cfTypeID` for checking whether `N…
norio-nomura Jun 2, 2017
070cf25
Update test_objCType
norio-nomura Jul 2, 2017
358b4f3
Explicitly declare `_Factory` protocol as `internal`
norio-nomura Jul 2, 2017
b19da15
Change `NSNumber` to initialize with `CFNumberCreate()`
norio-nomura Jul 2, 2017
0592ccf
Fix `NSKeyedUnarchiver.decodeBool(forKey:)` and add test to `TestNSPr…
norio-nomura Jun 3, 2017
b9fa1ac
Stop using `@_silgen_name`
norio-nomura Jul 2, 2017
bb182da
Remove stored instance properties from `NSNumber`
norio-nomura Jul 2, 2017
5a26410
`_CFNumberGetType2` only emit `SInt*` and `Float*` types
norio-nomura Jul 3, 2017
829de81
Revert "Remove stored instance properties from `NSNumber`"
norio-nomura Jul 3, 2017
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
2 changes: 2 additions & 0 deletions CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ CF_PRIVATE void _CFSwiftRelease(void *_Nullable t);

CF_EXPORT void _CFRuntimeBridgeTypeToClass(CFTypeID type, const void *isa);

CF_EXPORT CFNumberType _CFNumberGetType2(CFNumberRef number);

typedef unsigned char __cf_uuid[16];
typedef char __cf_uuid_string[37];
typedef __cf_uuid _cf_uuid_t;
Expand Down
4 changes: 3 additions & 1 deletion Foundation/JSONEncoder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
//
//===----------------------------------------------------------------------===//

import CoreFoundation

//===----------------------------------------------------------------------===//
// JSON Encoder
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -1742,7 +1744,7 @@ extension _JSONDecoder {
}

// TODO: Add a flag to coerce non-boolean numbers into Bools?
guard number._objCType == .Bool else {
guard number._cfTypeID == CFBooleanGetTypeID() else {
throw DecodingError._typeMismatch(at: self.codingPath, expectation: type, reality: value)
}

Expand Down
5 changes: 4 additions & 1 deletion Foundation/NSCFBoolean.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ internal class __NSCFBoolean : NSNumber {
override var objCType: UnsafePointer<Int8> {
// This must never be fixed to be "B", although that would
// cause correct old-style archiving when this is unarchived.
return UnsafePointer<Int8>(bitPattern: UInt(_NSSimpleObjCType.Char.rawValue.value))!
func _objCType(_ staticString: StaticString) -> UnsafePointer<Int8> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this trick better than the way I was thinking, however I wonder if we have a guarantee this is safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't the static string need to be a static property to guarantee its lifetime?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not fully certain which way this falls, constant strings should emit static references in the binary so it is probably safe-ish?

We have a few alternatives; we could thunk out to C and use the real @encode since we always have clang available. We could thunk out to C and reference C strings as opaque memory blobs. This approach. Or the other thought I had was something like this https://gist.github.com/phausler/2630092f1b391d6a5db0a562288514bf

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking along the same lines, but simpler:

fileprivate let boolEncoding: StaticString = "B"
fileprivate let boolEncodingPtr: UnsafePointer<Int8> = {
  precondition(boolEncoding.isASCII)
  return UnsafeRawPointer(boolEncoding.utf8Start).assumingMemoryBound(to: Int8.self)
}()

// etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, I think there is also value in abstracting it out to an isolated file to do that work in and provide a shared internal (minimal) version of @encode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discarded following codes that rewrote _NSSimpleObjCType in NSObjCRuntime.swift, since memory layout is not compatible with current implementation.
https://gist.github.com/norio-nomura/a1383f258c38bb1e081597a538f46e8f

Should we create another static values for @encode?
I feel we should do re-writing _NSSimpleObjCType instead of adding them, and doing that with this PR will make it too large.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I tested, utf8Start points directly cstring in __TEXT,__cstring,cstring_literals whether StaticString is instantiated as static variable or function parameter.

return UnsafeRawPointer(staticString.utf8Start).assumingMemoryBound(to: Int8.self)
}
return _objCType("c")
}

internal override func _cfNumberType() -> CFNumberType {
Expand Down
4 changes: 2 additions & 2 deletions Foundation/NSJSONSerialization.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,8 @@ private struct JSONWriter {
throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.propertyListReadCorrupt.rawValue, userInfo: ["NSDebugDescription" : "Number cannot be infinity or NaN"])
}

switch num._objCType {
case .Bool:
switch num._cfTypeID {
case CFBooleanGetTypeID():
serializeBool(num.boolValue)
default:
writer(_serializationString(for: num))
Expand Down
4 changes: 2 additions & 2 deletions Foundation/NSKeyedUnarchiver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,10 +683,10 @@ open class NSKeyedUnarchiver : NSCoder {
}

open override func decodeBool(forKey key: String) -> Bool {
guard let result : NSNumber = _decodeValue(forKey: key) else {
guard let result : Bool = _decodeValue(forKey: key) else {
return false
}
return result.boolValue
return result
}

open override func decodeInt32(forKey key: String) -> Int32 {
Expand Down
Loading