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.

Sign up for GitHub

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

Jump to bottom

BatArray.remove_at #996

Merged
merged 1 commit into from
Dec 23, 2020
Merged

BatArray.remove_at #996

merged 1 commit into from
Dec 23, 2020

Conversation

UnixJunkie
Copy link
Member

No description provided.

@UnixJunkie
Copy link
Member Author

related to #995

@UnixJunkie UnixJunkie merged commit 0ae3d5d into ocaml-batteries-team:master Dec 23, 2020
@UnixJunkie UnixJunkie deleted the array_remove_at branch December 23, 2020 02:30
rixed
rixed reviewed Dec 23, 2020
View reviewed changes
src/batArray.mlv
let remove_at i src =
let x = src.(i) in (* keep the bound check in there *)
let n = length src in
let dst = make (n - 1) x in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: In theory, Array.init would permit to initialize dst only once taher than twice (first with x, second with actual values). In practice IIRC it's the same under the hood.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this.
If it's faster, why not.
I think it might also make the code more readable.
Thanks for the review by the way.

@fccm fccm mentioned this pull request Jan 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rixed rixed rixed left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@UnixJunkie @rixed