search

byteverse / contiguous

Typeclass for array types
Other
19 stars 9 forks source link

Allow shrinking in place #62

Closed DaveBarton closed 6 months ago

DaveBarton commented 6 months ago

sizeofSmallMutableArray is deprecated like sizeofMutablePrimArray, so please use getSizeofSmallMutableArray (like you use getSizeofMutablePrimArray).

Then note shrinkMutablePrimArray and shrinkSmallMutableArray shrink in place, and resizeMutablePrimArray and resizeSmallMutableArray can resize in place (if the array shrinks, not grows). Thus it would be nice to have functions to shrink, resize, and shrinkAndFreeze that work in place if possible (if the array shrinks not grows, and currently if it's a mutable prim or small array). These could be called fastShrink/fastResize/unsafeFastShrinkAndFreeze, or unsafeShrink/unsafeResize/unsafeShrinkAndFreeze2. Actually we could just keep the name unsafeShrinkAndFreeze and allow it to work in place, though this is a breaking change.

Finally, unfoldrMutableN could use this new fastShrink/unsafeShrink.

I could prepare a PR if you want, if you tell me which names you like.

Thanks for the very useful library! For instance, in computer algebra one often has algorithms that work the same for various kinds of arrays, e.g. PrimArrays over the integers mod p for various sizes of p, or regular Arrays.

andrewthad commented 6 months ago

I agree that switching to getSizeofSmallMutableArray would be good. I would take a PR for that.

The existing functions shrink, resize, and unsafeShrinkAndFreeze already shrink in place for types where it's possible to do so. They new functions you propose have the exact behavior that the existing functions have.

DaveBarton commented 6 months ago

Ouch, you are right for PrimArray! But for SmallArray, the code is: resize = resizeSmallArray, where resizeSmallArray is defined in Data.Primitive.Contiguous.Shim and does copying. I think this needs to be resize = resizeSmallMutableArray instead. Also I'd document that the functions can shrink in place.

Do you agree? And if so, do you still want a PR, or do you just want to make the changes yourself? (Either way is fine with me.)

Thanks very much for the library, and your time!

andrewthad commented 6 months ago

I've made this change along with a few other improvements in https://github.com/byteverse/contiguous/pull/63. Would you be able to review it?

DaveBarton commented 6 months ago

Yes, thanks! It's my first github review so I'm going slowly. :)

DaveBarton commented 4 months ago

Could you make a release with this fix please? Thanks!!