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

[SR-929] Implement NSString.init() with locale #286

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Projects
None yet
4 participants
@seabaylea
Collaborator

seabaylea commented Mar 15, 2016

SR-929 covers the need to implement the following NSString API:

public convenience init(format: String, locale: AnyObject?, arguments argList: CVaListPointer)

This PR adds an implementation that:

  • Allows the for locale parameter to be a NSLocale, NSDictionary or nil.
    The parameter type is checked, checking NSLocale first as its the expected type
  • Casts the type to CFDictionary for use in CFString
    Here I've used unsafeBitCast as casting to a CFDictionary directly doesn't work on Linux and there doesn't appear to be a "safer" was of casting that works. The underlying functions check to see if the actual type is a CFLocale or a CFDictionary and handles as appropriate.
  • Throws a fatalError() for invalid locale parameters
    The actual behaviour is undefined by the spec. As the init function isn't failable there needs to be some kind of error, and it looked better to fail here with a relevant message than further down the call stack.

The behaviour is the same as for Objective-C on Mac, except for the error handling. Let me know if there's any suggest changes to the implementation.

I've also added 4 tests for the implementation:

  1. Checking the use of nil
  2. Checking with the English locale using NSLocale, returning 1,000 (42.0)
  3. Checking with the German locale using NSLocale, returning 1.000 (42,0)
  4. Checking with a custom decimal separator using a NSDictionary, returning 1000 (42&0)
@parkera

This comment has been minimized.

Show comment
Hide comment
@parkera

parkera Mar 16, 2016

Member

@swift-ci please test

Member

parkera commented Mar 16, 2016

@swift-ci please test

@parkera

This comment has been minimized.

Show comment
Hide comment
@parkera

parkera Mar 16, 2016

Member

Hm, looks like we've got some Swift 3 cleanup still to do in our test cases...

Member

parkera commented Mar 16, 2016

Hm, looks like we've got some Swift 3 cleanup still to do in our test cases...

@seabaylea

This comment has been minimized.

Show comment
Hide comment
@seabaylea

seabaylea Mar 16, 2016

Collaborator

I'll make the changes and update this PR with them.

Collaborator

seabaylea commented Mar 16, 2016

I'll make the changes and update this PR with them.

@seabaylea

This comment has been minimized.

Show comment
Hide comment
@seabaylea

seabaylea Mar 16, 2016

Collaborator

@parkera updated and squashed in place.

Collaborator

seabaylea commented Mar 16, 2016

@parkera updated and squashed in place.

} else {
str = CFStringCreateWithFormatAndArguments(kCFAllocatorSystemDefault, nil, format._cfObject, argList)
}
self.init(str._swiftObject)

This comment has been minimized.

@phausler

phausler Mar 18, 2016

Member

The one possible issue I could see with this implementation is that it wont call the description with locale on the NS objects: _CFStringCreateWithFormatAndArgumentsAux2 can pass a function pointer to allow for fetching the descriptionWithLocale() off of the objects. However that would mean that we would need to have an internal protocol to identify those classes who respond to that method.

@phausler

phausler Mar 18, 2016

Member

The one possible issue I could see with this implementation is that it wont call the description with locale on the NS objects: _CFStringCreateWithFormatAndArgumentsAux2 can pass a function pointer to allow for fetching the descriptionWithLocale() off of the objects. However that would mean that we would need to have an internal protocol to identify those classes who respond to that method.

This comment has been minimized.

@seabaylea

seabaylea Mar 21, 2016

Collaborator

Ok - so we need to pass a function pointer to _CFStringCreateWithFormatAndArgumentsAux2() for a function that's capable of calling any implementation of descriptionWithLocale() on objects passed in as part of the argList. Does this only apply to CF/NS objects, you do we need to handle 'plain' Swift objects as well?

@seabaylea

seabaylea Mar 21, 2016

Collaborator

Ok - so we need to pass a function pointer to _CFStringCreateWithFormatAndArgumentsAux2() for a function that's capable of calling any implementation of descriptionWithLocale() on objects passed in as part of the argList. Does this only apply to CF/NS objects, you do we need to handle 'plain' Swift objects as well?

This comment has been minimized.

@phausler

phausler Mar 21, 2016

Member

Technically on darwin the only reason why you can pass a swift class to this function is because they have root objc inheritance (that has the right methods implemented). However you cannot pass a struct to it even if it does conform to the descriptionWithLocale or the CustomStringConvertible protocol and I doubt there is any real way we can get struct passing to va_args to work in swift on either platform for formatting.

As it stands this implementation looks like a good start, if you want to make a bug on bugs.swift.org and have the format callbacks to be a later followup that is perfectly reasonable.

@phausler

phausler Mar 21, 2016

Member

Technically on darwin the only reason why you can pass a swift class to this function is because they have root objc inheritance (that has the right methods implemented). However you cannot pass a struct to it even if it does conform to the descriptionWithLocale or the CustomStringConvertible protocol and I doubt there is any real way we can get struct passing to va_args to work in swift on either platform for formatting.

As it stands this implementation looks like a good start, if you want to make a bug on bugs.swift.org and have the format callbacks to be a later followup that is perfectly reasonable.

This comment has been minimized.

@phausler phausler merged commit 141270e into apple:master Mar 30, 2016

@gribozavr

This comment has been minimized.

Show comment
Hide comment
@gribozavr

gribozavr Mar 30, 2016

Collaborator

Sorry, but it looks like there is a CI failure: https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-15_10/3818/console

Could someone investigate or revert for now?

Collaborator

gribozavr commented Mar 30, 2016

Sorry, but it looks like there is a CI failure: https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-15_10/3818/console

Could someone investigate or revert for now?

@phausler

This comment has been minimized.

Show comment
Hide comment
@phausler

phausler Mar 30, 2016

Member

actually it looks like the test is incorrect. will push a fix shortly

Member

phausler commented Mar 30, 2016

actually it looks like the test is incorrect. will push a fix shortly

@seabaylea seabaylea deleted the seabaylea:nsstring-init branch May 11, 2016

millenomi pushed a commit to millenomi/swift-corelibs-foundation that referenced this pull request Jan 8, 2018

Merge pull request #286 from compnerd/major-update
linux: update header used for `major` macro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment