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

WIP: Added standard module functions for ImmutableArray for FSharp.Core #15437

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

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
Member

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