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

Implemented functions for FlatList module to correspond to Array module #10

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

rokklobster
Copy link

No description provided.

src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperiandri xperiandri left a comment

Choose a reason for hiding this comment

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

В целом хорошо.
Сравни как в модуле List там кажись все фкнции принимают строго list но не seq.
Соответственно тебе нужно будет добавить аннотации

src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperiandri xperiandri left a comment

Choose a reason for hiding this comment

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

Ну раз raiseOrReturn уже устанавливает такое ограничение, то супер

src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
- remove excessive `get`
- add initialization with value (and use in `create`/`replicate`)
- use `Seq` in majority of functions
- simplify `fill`
- fix signatures
- change `countBy`, `indexed`, `iter2` to use `Seq`
- fix distinctBy (remove excessive arg + use `raiseOrReturn`)
Copy link
Collaborator

@xperiandri xperiandri left a comment

Choose a reason for hiding this comment

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

А так всё супер

src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
@xperiandri xperiandri changed the title wip: implementation of collection methods for flat list Implemented functions for FlatList module to correspond to Array module May 21, 2021
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
src/FSharp.Collections.Immutable/flat-list.fs Outdated Show resolved Hide resolved
Comment on lines 497 to 507
[<CompiledName("Build")>]
let inline build f =
let builder = builder()
f builder
moveFromBuilder builder

[<CompiledName("Update")>]
let inline update f list =
let builder = toBuilder list
f builder
moveFromBuilder builder
Copy link
Collaborator

Choose a reason for hiding this comment

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

А что эти функции делают?

Copy link
Author

Choose a reason for hiding this comment

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

Позволяют выполнить действие над билдером. Использований в модуле нет, думаю удалить их

@xperiandri
Copy link
Collaborator

Надо будет все вызовы переделать на вызовы отсюда
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections.Immutable/src/System/Linq/ImmutableArrayExtensions.cs

У них это единственный класс, который имеет свой LINQ.

@xperiandri
Copy link
Collaborator

Потому, что вместо проверки на null там проверяется, есть ли внутри ImmutableArray сам массив.
Соответственно почти везде проверки check можно будут повыбрасывать

@@ -16,49 +19,75 @@ module FlatList =
let inline internal checkNotDefault argName (list : FlatList<'T>) =
if list.IsDefault then invalidArg argName "Uninstantiated ImmutableArray/FlatList"
let inline internal check (list : FlatList<'T>) = checkNotDefault (nameof list) list
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/54c717a4ed822f46a23893479b8d4398596c041d/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Minimal.cs#L409

Получается самый быстрый вариант – это дёрнуть IsEmpty и выбросить
https://github.com/dotnet/runtime/blob/54c717a4ed822f46a23893479b8d4398596c041d/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Minimal.cs#L157

И получится то же, что они делают. Тогда не нужно использовать их LINQ обёртку

@@ -16,49 +19,75 @@ module FlatList =
let inline internal checkNotDefault argName (list : FlatList<'T>) =
if list.IsDefault then invalidArg argName "Uninstantiated ImmutableArray/FlatList"
let inline internal check (list : FlatList<'T>) = checkNotDefault (nameof list) list
let inline internal checkEmpty (list : FlatList<_>) = check list; if list.Length = 0 then invalidArg (nameof list) "Source is empty" else ()
let inline internal raiseOrReturn list = check list; list
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут наверное надо переименовать будет на checkAndReturn чтобы было в едином стиле

@xperiandri
Copy link
Collaborator

Посмотрел ещё раз https://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections.Immutable/src/System/Linq/ImmutableArrayExtensions.cs
По хорошему надо таки его использовать. Там много оптимизаций под ImmutableArray

@xperiandri
Copy link
Collaborator

При чём, наверное, именно для этой коллекции стоит отойти от правила возвращать такой же тип при преобразовании, а возвращать IEnumerable

- rename checkAndReturn
- use ImmutableArrayExtensions in some places
- remove ofSeq calls
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

Successfully merging this pull request may close these issues.

2 participants