Skip to content

libcollections: Add append_all_move method to Vec<T>. #15532

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

Closed
wants to merge 3 commits into from

Conversation

nham
Copy link
Contributor

@nham nham commented Jul 8, 2014

This adds a new method, append_all_move, to Vec. It's similar to push_all_move, but returns the current vector so it can be used again.

@lilyball
Copy link
Contributor

lilyball commented Jul 8, 2014

So far we have append() and append_one() that behave in this same way. I think that's sufficient precedent for an append_all_move(), although since append() is the equivalent of push_all() then it should probably be named append_move().

That said, I'm a bit leery about adding more stuff to Vec that's literally the existing stuff done in a functional style. What's next, a function version of reverse()? How about sort_by()?

Perhaps it's better to investigate an alternative approach. Once we have by-value closures, I would suggest something like

pub fn with(mut self, f: |&mut Vec<T>|) -> Vec<T> {
    f(&mut self);
    self
}

This will abstract away all the mutating methods so they can work in a functional style. So instead of return_a_vec().append(slice) you'd say return_a_vec().with(|v| v.push_all(slice)).

Without by-value closures, this would work for things like that push_all() above, but it won't work for e.g. your append_all_move(). But by-value closures are coming soon.

@nham
Copy link
Contributor Author

nham commented Jul 11, 2014

I've renamed it to append_move. Even with such a with function, I think this would be nice to have. The operation is common enough and return_a_vec().append_move(slice) looks nicer than return_a_vec().with(|v| v.push_all_move(slice))

@Kimundi
Copy link
Member

Kimundi commented Jul 11, 2014

Hm, a few random thoughts:

  • I'd very much like to see any append/push method that clones from a slice be removed and replaced with something that just takes a iterator. More composable, and less API duplication. But it might need something like a iterable trait to be ergonomical.
  • In the same way, I'd very much like to see all the append methods go away and be expressed through some composable way with push() (or with the iterator extend()). Again, the current way duplicates API for a different use case that exists for all types in Rust.
  • with might solve that and could be a generic trait impl for all types, but that might be overkill.

@alexcrichton
Copy link
Member

Closing due to in activity, but I think that we'll definitely shake out some conventions for methods like this during API triage.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
…-formatting, r=Veykril

On type format '(', by adding closing ')' automatically

If I understand right, `()` can surround pretty much the same `{}` can, so add another on type formatting pair for convenience: sometimes it's not that pleasant to write parenthesis in `Some(2).map(|i| (i, i+1))` cases and I would prefer r-a to do that for me.

One note: currently, https://github.com/rust-lang/rust-analyzer/blob/b06503b6ec98c9ed44698870cbf3302b8560b442/crates/rust-analyzer/src/handlers/request.rs#L357 fires always.
Should we remove the assertion entirely now, since apparently things work in release despite that check?
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

Successfully merging this pull request may close these issues.

4 participants