search

byteverse / contiguous

Typeclass for array types
Other
18 stars 8 forks source link

fix the replicate story #19

Closed chessai closed 5 years ago

chessai commented 5 years ago

now we have replicate, replicateMutable, and replicateM in Contiguous.

I feel that this is more consistent with the rest of the ecosystem.

chessai commented 5 years ago

@andrewthad what are your thoughts on this and the related issue?

chessai commented 5 years ago

Related issue is #8

andrewthad commented 5 years ago

This PR is mostly good but still has some things a little bit off. Notably, replicateM is misnamed. I think the names we actually want are:

replicate :: (Contiguous arr, Element arr b) => Int -> b -> arr b
replicateMutable :: (Contiguous arr, PrimMonad m, Element arr b) => Int -> b -> m (Mutable arr (PrimState m) b)
replicateMutableM :: (Contiguous arr, PrimMonad m, Element arr b) => Int -> m b -> m (Mutable arr (PrimState m) b)

Now, the other important question is which ones of these actually need to be typeclass methods. We need replicateMutable in the typeclass because array initialization works differently for the lifted and unlifted array types. However, replicate n x = runST (replicateMutable n x >>= unsafeFreeze), which cannot be improved upon for any of the array types, so it doesn't need to be in there. We can kick out replicateMutableM as well since it is always implemented as: 1. create an array of unitialized elements 2. loop with the monadic generator to produce each element.

chessai commented 5 years ago

suggested changes have been made