Skip to content
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

[TODO] set inconsistencies #7556

Closed
3 of 4 tasks
timotheecour opened this issue Apr 10, 2018 · 11 comments
Closed
3 of 4 tasks

[TODO] set inconsistencies #7556

timotheecour opened this issue Apr 10, 2018 · 11 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Apr 10, 2018

  • ['a', 'b'].toSet returns a HashSet[char], not a set[char]
  • ['a', 'b'].toHashSet is not defined
  • card is defined for both set and HashSet, but len is defined for HashSet only, not set
  • echo({'a', 'b', 'c', 'b'}.card) : 4 (that one is already reported here: set bug: echo({'a', 'b', 'c', 'b'}.card) : 4 #7555)
@cooldome
Copy link
Member

I can agree only with point 3 on len

@narimiran
Copy link
Member

Number 4 was resolved a day ago by #7558.
len and card in HashSets are the same function with a different name - number 3 could be easily solved by aliasing.

That leaves first two points to consider/solve.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 11, 2018

/cc @narimiran

That leaves first two points to consider/solve.

Well assuming number 3 could be easily solved by aliasing is done :-)

actually i don't even know why we need card at all, seems like pointless aliasing that introduces a new symbol ; len is what's used everywhere else. Why not mark card as deprecated?

@krux02
Copy link
Contributor

krux02 commented Apr 17, 2018

@timotheecour you could make it a pr that uses the deprecated pragma, I would vote for it.

@metagn
Copy link
Collaborator

metagn commented Apr 17, 2018

The deprecated pragma doesn't work for procs.

set and HashSet are different things, set is not a linear type, as in you can't do for i in 0..<x.len: echo x[i] easily. HashSet is a length attribute and a seq of hashes and elements, so it's easy to know that its length is the length of the seq it uses, but set has a different implementation that might make the term "length" misleading.

@andreaferretti
Copy link
Collaborator

The length of a set is essentially one or more popcount operations, which are exposed in bitops, so it seems to me not really problematic

@narimiran
Copy link
Member

Not an inconsistency between sets and HashSets, but while we're talking about what should be implemented:

The thing missing from both versions is pop, which should remove and return a random element from a (hash)set.

@krux02
Copy link
Contributor

krux02 commented Apr 17, 2018

Well I disagre on pop.

@Araq
Copy link
Member

Araq commented Sep 2, 2018

I think the rest is fine, at one point you have to stop whining and read some documentation.

@Araq Araq closed this as completed Sep 2, 2018
@timotheecour timotheecour changed the title set inconsistencies [TODO] set inconsistencies Sep 3, 2018
@timotheecour
Copy link
Member Author

#11885 fixed item 3 in top post

@timotheecour
Copy link
Member Author

remaining item in this list would be fixed by merging #16276 and removing the (now deprecated) std/sets.toSet.

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

No branches or pull requests

7 participants