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

std: add std.ArrayList.remove #2262

Merged
merged 1 commit into from
May 3, 2019
Merged

Conversation

daurnimator
Copy link
Contributor

Closes #2247

Required for a sensible http headers object.

@andrewrk
Copy link
Member

I'm on board with adding this function, but since it has an important caveat of O(N) performance, I'd like to request the following changes:

  • a name that makes it sound like not one of the "main" operations of ArrayList. For example removeElementByShifting. I'm open to other names as well.
  • add documentation comments for the function mentioning O(N) and explaining how the function is implemented.

@andrewrk andrewrk added the work in progress This pull request is not ready for review yet. label Apr 29, 2019
@daurnimator
Copy link
Contributor Author

I was thinking that it would be good if zig container types all had the same API (for operations they support), with big-O behaviour documented in e.g. a table. Similar to C++

.remove is already supported by

  • hash_map
  • priority_queue
  • rb
  • linked_list
  • atomic/queue

@andrewrk
Copy link
Member

I was thinking that it would be good if zig container types all had the same API (for operations they support)

Please take this question earnestly - I'm not trying to be condescending - can you explain this reasoning? Other than a vague sense of tidiness that one gets from cleaning their room.

@vgmind
Copy link

vgmind commented Apr 30, 2019

I guess being explicit here is important as there is more than one valid way to remove an element from an ArrayList. There's the shift method being implement here that has certain run time characteristics that people with large arrays may not like but keeps the original order. Then there is the swap with the last element and just decrease the size, not great if you need to keep your Array list ordered but has constant performance characteristics.

@Tetralux
Copy link
Contributor

Tetralux commented May 2, 2019

See #2247 (comment).

@andrewrk
Copy link
Member

andrewrk commented May 3, 2019

@daurnimator you good with orderedRemove?

@daurnimator
Copy link
Contributor Author

@daurnimator you good with orderedRemove?

I'm okay with it; I'd still like to call it .remove, I've been meaning to do a write up on why; though we can make a new issue for that if we can merge this now?

@andrewrk andrewrk merged commit fca3e3a into ziglang:master May 3, 2019
@daurnimator daurnimator deleted the arraylist-remove branch May 3, 2019 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress This pull request is not ready for review yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add std.ArrayList.remove
4 participants