Add Belt_Array.partition #2825
Add Belt_Array.partition #2825
Conversation
jscomp/others/belt_Array.ml
Outdated
| ) | ||
| done; | ||
| let a1 = slice a1 ~offset:0 ~len:!i in | ||
| let a2 = slice a2 ~offset:0 ~len:!j in |
There was a problem hiding this comment.
instead of doing another copy using slice, would set the length (using the js trick) directly help?
There was a problem hiding this comment.
Ah. I thought the same, and was looking for a way of doing this in OCaml, but couldn't find any. Could you point me in the right direction? Use external?
I see there's a truncateToLengthUnsafe. Didn't catch that. Updating now
jscomp/others/belt_Array.ml
Outdated
| let a2 = makeUninitializedUnsafe l in | ||
| for ii = 0 to l - 1 do | ||
| let v = getUnsafe a ii in | ||
| if f v then ( |
There was a problem hiding this comment.
This introduced a currying in the loop, can you rename partition into partitionU and call f v [@bs] here (you can check other examples)
There was a problem hiding this comment.
I also see I've messed up the order of the arguments. Guess it's intended to use with e.g. fast pipe. I've updated this and the currying now. Does it look OK?
|
@bobzhang Thanks for the feedback. Did some changes now |
|
@mikaelbr looks great, thanks |
This intends to fix #2753.
I've tried do to an implementation of
Array.partitionusing the same style as the existing file (with unsafe set/get and refs). I tried multiple solutions for implementingpartition, trying to make it efficient in space and time (insert tardis joke), but I didn't see a way of reducing it.I've seen some other implementations as well, and they all do it in like 3-4 loops. So as far as I can think, this is as good as it gets? If anyone has any ideas, please let me know and I'll try to fix them.
Also please let me know if there is anything I can change with this PR.