CBOR icon indicating copy to clipboard operation
CBOR copied to clipboard

Efficient way to pass in immutable/non-array collections

Open charlesroddie opened this issue 4 years ago • 2 comments

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 avatar Aug 09 '21 18:08 charlesroddie

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.

charlesroddie avatar Nov 09 '23 16:11 charlesroddie

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.

peteroupc avatar Nov 09 '23 18:11 peteroupc