Skip to content

Conversation

@vzarytovskii
Copy link
Member

Based on the approved RFC - https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1094-immarray.md

A core library counterpart for the #12859

This is still missing:

  • Unit tests for Microsoft.FSharp..Collections.ImmutableArray
  • Codegen tests for Microsoft.FSharp..Collections.ImmutableArray (in particular around inlining).
  • RFC with functions list
  • FSI file for the collection
  • Anything else?

@vzarytovskii vzarytovskii requested a review from a team as a code owner June 19, 2023 15:33
@vzarytovskii vzarytovskii marked this pull request as draft June 19, 2023 15:33
let inline concat (arrs: IEnumerable<ImmutableArray<'T>>) : ImmutableArray<'T> =
match arrs with
| :? ImmutableArray<ImmutableArray<'T>> as arrs -> concatImmutableArrays arrs
| arrs -> concatImmutableArrays (ImmutableArray.CreateRange(arrs))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you include the body of concatImmutableArrays in this function then collect can call concat. Which eliminates a wierd public api: concatImmutableArrays

@T-Gro
Copy link
Member

T-Gro commented Jun 19, 2023

TODO: Include ImmutableArray into existing property tests which exercise consistency between List/Array/Seq/ArrayParallel

(this will also show if the main functions are covered or not)

@T-Gro
Copy link
Member

T-Gro commented Jun 19, 2023

TODO: sorting?

@vzarytovskii
Copy link
Member Author

TODO: sorting?

Been thinking about it, array's sorting is special, since it's in place.
Which (defaults) shall we include here?

@vzarytovskii
Copy link
Member Author

TODO: Include ImmutableArray into existing property tests which exercise consistency between List/Array/Seq/ArrayParallel

(this will also show if the main functions are covered or not)

Do you remember location from the top of your head?

@T-Gro
Copy link
Member

T-Gro commented Jun 19, 2023

module FSharp.Core.UnitTests.Collections.CollectionModulesConsistency

@T-Gro
Copy link
Member

T-Gro commented Jun 19, 2023

TODO: sorting?

Been thinking about it, array's sorting is special, since it's in place. Which (defaults) shall we include here?

I think that at least sortBy would be nice, otherwise user code would have to (even though it is what this impl will most likely do anyway..):
unwrap into array
sort that in place
pack back into immarray

on their own.

@nojaf
Copy link
Contributor

nojaf commented Jun 20, 2023

Anything else?

You probably want to remove https://github.com/dotnet/fsharp/blob/cb106cf3182ff218f0a0e42780815dba94b60013/src/Compiler/Utilities/ImmutableArray.fs and use the new additions in F# Core.

And will the 'T immarray alias be introduced as well in F# Core?

@vzarytovskii
Copy link
Member Author

Anything else?

You probably want to remove https://github.com/dotnet/fsharp/blob/cb106cf3182ff218f0a0e42780815dba94b60013/src/Compiler/Utilities/ImmutableArray.fs and use the new additions in F# Core.

No, not yet, this will break all scenarios where FSHARPCORE_USE_PACKAGE is used.

And will the 'T immarray alias be introduced as well in F# Core?

In separate PR, yes. This one is only about module functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

4 participants