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

Efficient way to pass in immutable/non-array collections #55

Open
charlesroddie opened this issue Aug 9, 2021 · 2 comments
Open

Efficient way to pass in immutable/non-array collections #55

charlesroddie opened this issue Aug 9, 2021 · 2 comments

Comments

@charlesroddie
Copy link
Contributor

charlesroddie commented Aug 9, 2021

CBORObject.FromObject has an overload taking arrays:

    public static CBORObject FromObject(CBORObject[] array) {
      if (array == null) {
        return CBORObject.Null;
      }
      IList<CBORObject> list = new List<CBORObject>();
      foreach (CBORObject cbor in array) {
        list.Add(cbor);
      }
      return new CBORObject(CBORObjectTypeArray, list);
    }

There is an additional collection created here: since array is mutable it cannot be trusted to be used directly, so it is duplicated into an List.

Passing in collections other than arrays take a further collection creation, as they first need to be converted to arrays since that is the only overload.

It would be good to have an efficient way to pass in an ImmutableArray<CBORObject>, or one of the interfaces that it supports: ICollection<T>, IEnumerable<T>, IList<T>, IReadOnlyCollection<T>, IReadOnlyList<T>, IImmutableList<T>.

This could be done by taking an IEnumerable<T> and using the current approach. That would only be one collection creation, copying to a List.

If it used one of the immutable interfaces internally (ImmutableArray, IImmutableList), then no copying would be needed, as the input could be used directly without risk of changes.

@charlesroddie
Copy link
Contributor Author

I started on replacing the mutable List with ImmutableArray and encountered the problem that there are currently mutation operators on CBORObject. These are designed to Insert or Delete etc. if the CBORObject is a list. To me these methods shouldn't be there because 1. a CBORObject is otherwise not mutable and has no reason to be mutable, and 2. they don't respect the CBORObject type, trying to bring information from elsewhere that the CBORObject in question is a list, so indicate a typing problem in consuming code where necessary information was lost.

Possible solutions (not including making the CBORObject's internal value mutable which would be terrible) are:

  1. remove the methods
  2. adjust the methods to return new CBORObjects (rather than, for example, a bool indicating whether the operation was successful.

Would either of these approaches be acceptable @peteroupc ? Both breaking changes.

@peteroupc
Copy link
Owner

The methods that would render CBORObject mutable could be deprecated, and there could be builder classes (such as CBORArrayBuilder and CBORMapBuilder) that could serve as mutable versions of CBOR arrays and maps.

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

2 participants