arrow-kt / arrow

Ξ›rrow - Functional companion to Kotlin's Standard Library
http://arrow-kt.io
Other
6.18k stars 452 forks source link

NonEmptySet: change 2nd param of constructor to `Iterable`. Optimize`nonEmptySetOf` and `toNonEmptySetOrNull` #3389

Closed hoc081098 closed 5 months ago

hoc081098 commented 8 months ago
serras commented 8 months ago

I am not really sure what is the optimization here. How are you ensuring that elements are not duplicated when using nonEmptySetOf?

Furthermore, note that we are soon going to make arrow-2 the new main branch. There NonEmptySet is defined differently, using an inline value class. It would be better if optimizations would be applied there, as we don't expect to make any further 1.2.x release.

hoc081098 commented 8 months ago

I am not really sure what is the optimization here. How are you ensuring that elements are not duplicated when using nonEmptySetOf?

In constructor, we have used setOf(first) + rest, it returns a Set.

hoc081098 commented 8 months ago
hoc081098 commented 8 months ago
  • Prev code: res (varargs === Array) -> toSet -> setOf(first) + res uses 2 loops internally.
  • After code : res (varargs === Array) -> asList -> setOf(first) + res uses 1 loops internally (because asList does not copy the array to List, but returns a read-only wrapper) πŸ™

I have changed the target branch to arrow-2 branch. @serras Please review it again πŸ™

serras commented 5 months ago

@hoc081098 sorry for the unnecesary noise but, would it be possible for you to rebase these changes into the new main? Otherwise it's impossible for us to merge properly this.