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

Allow shrinking in place #62

Closed
DaveBarton opened this issue Apr 23, 2024 · 5 comments
Closed

Allow shrinking in place #62

DaveBarton opened this issue Apr 23, 2024 · 5 comments

Comments

@DaveBarton
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

I've made this change along with a few other improvements in #63. Would you be able to review it?

@DaveBarton
Copy link
Contributor Author

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

DaveBarton added a commit to DaveBarton/contiguous that referenced this issue May 7, 2024
@DaveBarton
Copy link
Contributor Author

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

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

No branches or pull requests

2 participants