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

System.Collections.Generic ISet<T> and IDictionary<T> (Fixes #53) #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NightOwl888
Copy link

This PR addresses #53:

  • Implements SCG.ISet<T> on TreeSet<T>
  • Implements SCG.IDictionary<K, V> on DictionaryBase<K, V>

I also attempted to implement SCG.ISet<T> on HashSet<T>, but since it seems to be missing a way to get and delete items by index, I am not sure how to proceed. Here is the (non compiling) attempt, in case you want to try to complete it:

Redundant Behavior

Do note that the AddAll, ContainsAll RetainAll, and RemoveAll methods are now redundant with UnionWith, IsSupersetOf, IntersectWith, and ExceptWith, respectively.

Options

  1. Leave the redundant methods in place
  2. Break C5 backward compatibility, but follow .NET conventions by removing (or making internal) AddAll, ContainsAll RetainAll, and RemoveAll
  3. Preserve C5 backward compatibility, and hide UnionWith, IsSupersetOf, IntersectWith, and ExceptWith by using explicit interface declarations

Performance

Most of the ISet<T> methods were reverse engineered from SCG.HashSet<T>, but it may be possible to implement some of the methods at a lower level for better performance - I didn't look into it.

@sestoft
Copy link
Owner

sestoft commented Jan 2, 2020 via email

@NightOwl888
Copy link
Author

You mean, using the indexer set[i] = … where i is an integer?

Having this indexer on ISet is an abominable design mistake in the .Net library, and I think it should throw an exception on HashSet and similar. What does it do on SCG.HashSet?

No, what I meant was that the SCG.HashSet<T> has an IndexOfInternal(TKey) method and a way to delete an item internally by index. Since TreeSet<T> has an external IndexOf(TKey) and RemoveAt(int) methods, it was easy to translate. However, the C5 HashSet<T> doesn't seem to expose this functionality, so it is not as straightforward to implement ISet<T>.

@sestoft
Copy link
Owner

sestoft commented Jan 4, 2020 via email

@NightOwl888
Copy link
Author

Strange, I don’t see any (documentation of) such a method on either SCG.HashSet or SCG.ISet.

You won't find any documentation, because as I mentioned, the method is intenal:

https://github.com/microsoft/referencesource/blob/master/System.Core/System/Collections/Generic/HashSet.cs#L1423

There is also an internal way to delete a slot by index after determining which indices need removal:

https://github.com/microsoft/referencesource/blob/master/System.Core/System/Collections/Generic/HashSet.cs#L1310

In any case I believe the best way to implement it is by a method that throws a suitable exception, with a message such as “Not implemented”.

Since this is the slow path that happens whenever the equality comparers don't match (which is probably 70-80% of the time), it would mean that all of the set operations would throw an exception 70-80% of the time.

Of course, if there is no way to get the index of an item or delete an item by index, this could be implemented by wrapping each item in a structure with a bit to track the items, but that would be a lot slower than using a BitArray to track the indicies.

I was hoping you had a more clever lower-level way to complete this than what Microsoft did.

But whatever the case, to me it makes more sense to provide the basic tests and a direction to go than to provide a broken implementation or one that is unreasonably slow. Since it is not part of the PR, you can either use your knowledge of the inner workings of C5's HashSet to complete it the right way or completely leave it out.

@sestoft
Copy link
Owner

sestoft commented Jan 5, 2020 via email

@ondfisk
Copy link
Collaborator

ondfisk commented Jan 5, 2020

AddAll, ContainsAll, RetainAll, and RemoveAll come from ICollection or IExtensible.
They should maybe be called *Range to match the naming in .NET.
My thinking would be to rename *All to *Range and then go with option 3: hide by using explicit interface declarations. This is in line with how SCG.LinkedList<T> implements SCG.ICollection<T>.
WRT HashSet<T> I think we should provide a crude implementation of ISet<T> to allow it to be used and possibly not the the perf is not optimal for ISet<T> operations.

@NightOwl888
Copy link
Author

AddAll, ContainsAll, RetainAll, and RemoveAll come from ICollection or IExtensible.
They should maybe be called *Range to match the naming in .NET.
My thinking would be to rename *All to *Range and then go with option 3: hide by using explicit interface declarations. This is in line with how SCG.LinkedList implements SCG.ICollection.
WRT HashSet I think we should provide a crude implementation of ISet to allow it to be used and possibly not the the perf is not optimal for ISet operations.

Do you mean hide all of the ISet<T> set operations, or just the redundant ones?

It would be useful to provide the rest of the set operations (IsSubsetOf, IsProperSubsetOf, IsProperSupersetOf, and Overlaps) as top-level methods. However, it feels like a crime to provide IsSubsetOf, IsProperSubsetOf, IsProperSupersetOf and name the remaining one ContainsRange instead of IsSupersetOf.

Either way it feels like Overlaps should definitely be on the top level, but IsSubsetOf, IsProperSubsetOf, IsProperSupersetOf and IsSupersetOf should be all public or all hidden.

@ondfisk
Copy link
Collaborator

ondfisk commented Jan 5, 2020

No I mean hide the IExtensible methods so HashSet and TreeSet looks and behaves like SCG.ISet.
That’s a breaking change but I think it’ll make more sense than hiding the ISet methods.
We just need to ensure the C5 event stuff is implemented also.

@ondfisk
Copy link
Collaborator

ondfisk commented Jan 14, 2020

@NightOwl888: are you working on the changes mentioned above?

@NightOwl888
Copy link
Author

I haven't had a chance to get to it yet, and it might be awhile until I have the time. No objections if you want to jump ahead and do it yourself.

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