Skip to content

Implementation of NSCountedSet #88

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

Merged
merged 2 commits into from
Mar 1, 2016
Merged

Conversation

codestergit
Copy link
Contributor

This pull request contains the implementation of NSCountedSet. It includes implementation and test cases for NSCountedSet. Please let me know if you need any changes in this. Suggestion are welcome. I am not that much expert with system implementation. So please let me know your views or any other changes required.
Thanks to swift team

@codestergit codestergit changed the title [Need feedback]: Implementation of NSCountedSet Implementation of NSCountedSet Dec 8, 2015
@codestergit
Copy link
Contributor Author

@parkera Updated pull request to resolve the conflicts. Please have a look.
Thanks

@phausler
Copy link
Contributor

unfortunately this causes a SIL gen crasher on linux:

  1. While silgen constructor initializer SIL function @TFC10Foundation12NSCountedSetcfT5arrayGSaPs9AnyObject___S0 for 'init' at Foundation/NSSet.swift:407:24

@codestergit
Copy link
Contributor Author

Thanks @phausler . Do not have linux machine will try to arrange something and run the code. Thanks for reviewing. Do you know the reason? public convenience init(array: [AnyObject])

@phausler
Copy link
Contributor

I am not certain on why, we have had a few of these beasts lurking under the waters around NSSet and NSDictionary specifically when it comes to implementing initializers that take arrays

@codestergit
Copy link
Contributor Author

@phausler @parkera Getting the errors while building swift on ubuntu 15.10 http://pastebin.com/9b9ubyit . Any idea?

@parkera
Copy link
Contributor

parkera commented Dec 12, 2015

You'll need to make sure your Linux machine has lots (> 8 GB) of RAM in order for linking to finish. If it fails like that, you can also just try again.

@parkera
Copy link
Contributor

parkera commented Dec 12, 2015

Also, pass -R to the build script - the release version of clang/swift uses less memory to link.

@codestergit
Copy link
Contributor Author

Thanks @parkera !! Do not have that much RAM also running in VMWare :( . Will try to arrange somewhere else :(

@codestergit codestergit force-pushed the master branch 3 times, most recently from 434bca0 to 27a3b3d Compare December 16, 2015 19:03
@codestergit
Copy link
Contributor Author

@parkera @phausler Updated the pull request to resolve compiler bug. Run all the tests on linux ubuntu 15.10 and Mac. Please review it.
Thanks

@parkera
Copy link
Contributor

parkera commented Dec 18, 2015

The general idea here is ok, but I have one major concern. The implementation limits itself to holding subclasses of NSObject. This is actually not required even on Darwin:

class Foo { }

let f = Foo()
var x = NSCountedSet()
x.addObject(f)
x.addObject(f)
x.addObject(f)

x.count // 1
x.countForObject(f) // 3

This may be one area where we are again bumping up against the difficulties of lacking a bridging story while maintaining cross-platform source compatibility...

@codestergit
Copy link
Contributor Author

@parkera @phausler Updated the pull request to resolve conflicts. Verified test are passing. Please review it.
I think we need bridging for NSObject. NSMutableSet and other classes also have this limitation. It is very hard to make compatible with AnyObject as our storage Set needs to be NSObject or some Hashable implementation. We can not make Set to Set<Hashable> as Hashable is not concrete protocol. I think with type erasure also our api user need to know about this. After all this it will not give benefit of NSObject bridging as above case.
I think for now we can move forward with this. Please let me know your views.
Thanks

@codestergit
Copy link
Contributor Author

@phausler @parkera Updated the pull request to resolve conflicts. Please review the pull request. All tests are passing. Please let me know if any changes required.
Thanks

@codestergit
Copy link
Contributor Author

Hi @phausler
I have updated the pull request to resolve conflicts.
Please Review it.
Thanks.

@codestergit
Copy link
Contributor Author

@phausler It's pending for while. Can you please review it. If you need any changes please let me know.
Thanks

let v2 = "v2".bridge()
let v3asv1 = "v1".bridge()
let set = NSCountedSet()
let set2 = NSCountedSet(array: [v1, v1, v2,v3asv1].bridge().bridge())
Copy link
Contributor

Choose a reason for hiding this comment

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

the double bridge here is probably not needed since the elements in the Array are of type NSString which should fit as AnyObject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phausler Thanks. Done the changes.

@phausler
Copy link
Contributor

@parkera this looks good to me, do you have any outstanding issues here?

@codestergit
Copy link
Contributor Author

@phausler Updated the pull request to address the comments. Thanks for reviewing it.

phausler added a commit that referenced this pull request Mar 1, 2016
Implementation of NSCountedSet
@phausler phausler merged commit ebb1bcb into swiftlang:master Mar 1, 2016
@parkera
Copy link
Contributor

parkera commented Mar 1, 2016

Thanks!

atrick pushed a commit to atrick/swift-corelibs-foundation that referenced this pull request Jan 12, 2021
[editors] Add alternative swift syntax spec to sublime config
kateinoigakukun pushed a commit to kateinoigakukun/swift-corelibs-foundation that referenced this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants