SR-1464: NSNumber.description is not compatible between OS X and linux #1724
Conversation
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
| @@ -6013,7 +6013,7 @@ enum { | |||
| CFFormatDummyPointerType = 42 /* special case for %n */ | |||
| }; | |||
|
|
|||
| #if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_WINDOWS | |||
| #if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_WINDOWS || DEPLOYMENT_TARGET_LINUX | |||
There was a problem hiding this comment.
This applies to all patches, but: going forward, we need to start using TARGET_OS_* instead of DEPLOYMENT_*. I’m going to be pinging patches when they make changes to an if line that contains the old constants.
Can this be made to read TARGET_OS_MAC || TARGET_OS_WIN32 || TARGET_OS_LINUX?
There was a problem hiding this comment.
Yeah, thats no problem. Should the EMBEDDED be added as well ?
There was a problem hiding this comment.
TARGET_OS_MAC includes all Mac OS X-derived software, including what 'embedded' used to target. (You can replace 'embedded' with TARGET_OS_IPHONE.)
There was a problem hiding this comment.
_MAC includes _IPHONE and _OSX; _IPHONE includes _IOS, _WATCH etc.
There was a problem hiding this comment.
Wouldn't that make more sense if TARGET_OS_MAC becomes TARGET_OS_DARWIN? Technically Darwin is the operating system.
There was a problem hiding this comment.
The macros come from TargetConditionals.h in the macOS/iOS SDK. We're trying to maintain cross-platform standardization for their names.
There was a problem hiding this comment.
Thanks for clarification.
| @@ -7109,7 +7109,7 @@ static Boolean __CFStringAppendFormatCore(CFMutableStringRef outputString, CFStr | |||
| switch (specs[curSpec].type) { | |||
| case CFFormatLongType: | |||
| case CFFormatDoubleType: | |||
| #if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_WINDOWS | |||
| @@ -832,19 +832,17 @@ class TestNSString: LoopbackServerTest { | |||
| XCTAssertEqual(string, "Default value is 1000 (42.0)") | |||
| } | |||
|
|
|||
| #if false // these two tests expose bugs in icu4c's localization on some linux builds (disable until we can get a uniform fix for this) | |||
There was a problem hiding this comment.
Is this no longer applicable after this patch?
There was a problem hiding this comment.
I think enabling the __CFStringFormatLocalizedNumber makes it work on Linux now. I tested it on 14.04 and it seemed ok
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@millenomi Do these changes look ok now? |
|
@swift-ci test linux |
6 similar comments
|
@swift-ci test linux |
|
@swift-ci test linux |
|
@swift-ci test linux |
|
@swift-ci test linux |
|
@swift-ci test linux |
|
@swift-ci test linux |
- Use Swift's stringification of numbers for .description and .stringValue - Use String(format:locale:...) with the correct format for the NSNumber type if locale is not nil. - Enable use of __CFStringFormatLocalizedNumber() on DEPLOYMENT_TARGET_LINUX.
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@millenomi I think all your comments have been addressed now, do you see any other issues? |
|
@swift-ci please test and merge |
Use Swift's stringification of numbers for .description and .stringValue
Use String(format:locale:...) with the correct format for the
NSNumber type if locale is not nil.
Enable use of __CFStringFormatLocalizedNumber() on
DEPLOYMENT_TARGET_LINUX.