fix!: added plainly-callable Add() method to NetworkSet [MTT-1005]

[mattwalsh-unity]

Resolves issue #1006 and jira MTT-1005

[mattwalsh-unity]

Git has made a mess of this diff; I just hoisted the Add functionality out of the interface-qualified definitions into a method and then made those interfaces call that method.

[JesseOlmer]

2 nit comments, otherwise lgtm

[JesseOlmer]

nit: You don't need 3 methods for this. Just put the implementation in the ISet and call it from the ICollection.

[mattwalsh-unity]

I think the wackiness here is that we have ISet<T> which has its own Add method, and then ICollection<T> (which is a parent to ISet<T>) also has its own Add method, which differs only by return type, and so I have to implement both explicit interfaces.

But then if a user wants to call either one, they have to do a non-intuitive cast, e.g. ((ISet<int>)m_ServerComp.TheSet).Add(1234);. That's why I did the 3rd method. But maybe I'm missing another way?

I'm thinking about using a less grand interface than ISet / ICollection anyway vs. today where we promise to do everything those interfaces do, so that if we use native container underlying storage we don't have to make up for make of these calls that the native types don't support.

[0xFA11]

HashSet<T> is similar and it implements both ISet<T> and ICollection<T> at the same time — and yes, it implements both void returning and bool returning methods and they both call the exact same method behind the scenes. Let me get you some useful links to relevant points.

bool Add:

public bool Add(T item) => AddIfNotPresent(item, out _);

https://github.com/dotnet/runtime/blob/1a4675db08384e4f20cf4481e6eaf03e40f3d754/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L455

void Add:

void ICollection<T>.Add(T item) => AddIfNotPresent(item, out _);

https://github.com/dotnet/runtime/blob/1a4675db08384e4f20cf4481e6eaf03e40f3d754/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L181

underlying AddIfNotPresent (the actual code):

private bool AddIfNotPresent(T value, out int location)
{
    if (_buckets == null)
    {
        Initialize(0);
    }
    Debug.Assert(_buckets != null);

    Entry[]? entries = _entries;
// ...

https://github.com/dotnet/runtime/blob/1a4675db08384e4f20cf4481e6eaf03e40f3d754/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs#L1084

I think we could follow the same interface pattern closely to make our stuff more .NET-like :)

[JesseOlmer]

Yeah, I ended up sending Matt much the same thing in a DM & gist. The key here is you don't need explicit implementation on both of them, just enough to disambiguate. Explicit implementation of both is also what causes this required cast in the first place, as explicit implementations always need an explicit cast.

[JesseOlmer]

nit: Maybe use an expression-bodied method to make it SUPER clear this is just a remap... e.g.

bool Add(T item)
{
   ..
}

void ICollection<T>.Add(T item) => Add(item);

(you also only need the explicit implementation on one of them)

[mattwalsh-unity]

Yeah, cool, I can do that on ICollection, but I'm stuck on doing this for ISet because it has a different return type

[JesseOlmer]

My code snippet should work. You need to change your implementation to return a bool, and then just ignore it on ICollection, that should meet both ISet and ICollection needs (but is a moot point if you do the suggestion in the other comment thread)