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

proposal: x/exp/slices: add Pop #52434

Open
thatradius opened this issue Apr 19, 2022 · 8 comments
Open

proposal: x/exp/slices: add Pop #52434

thatradius opened this issue Apr 19, 2022 · 8 comments

Comments

@thatradius
Copy link

thatradius commented Apr 19, 2022

// Pop removes the elements s[i] from s, returning the modified slice
// and the removed element. Pop panics if s[i] is not a valid slice of s.
func Pop[S ~[]E, E any](s S, i int) (S, E) {
	e := s[i]
	return append(s[:i], s[i+1:]...), e
}
@gopherbot gopherbot added this to the Proposal milestone Apr 19, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 19, 2022
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Apr 19, 2022

Can you write a doc comment for the proposed function? Specifically, what happens when the slice is empty.

Seems odd to have Pop without Push.

@thatradius
Copy link
Author

thatradius commented Apr 19, 2022

@ianlancetaylor Like Delete but returns the removed element.

@thatradius
Copy link
Author

thatradius commented Apr 19, 2022

About Push maybe just like this.

// Push appends the element at s[:i], returning the modified slice.
func Push[S ~[]E, E any](s S, i int, e E) S {
	return append(append(s[:i], e), s[i+1:]...)
}

@seankhliao
Copy link
Member

seankhliao commented Apr 19, 2022

That Push is the same as Insert

Push/Pop seems like a strange name for the given actions, usually they mean operating at the ends of a queue

@thatradius
Copy link
Author

thatradius commented Apr 19, 2022

Maybe change the method name to Remove like Rust's vector?

@DeedleFake
Copy link

DeedleFake commented Apr 19, 2022

I agree that Pop is a strange name for it, but Remove for parity with Insert makes sense to me.

@thatradius
Copy link
Author

thatradius commented Apr 20, 2022

What about this?

// Pop removes the element s[i] from s, returning the modified slice
// and the removed element. Pop panics if i is not valid index of s.
func Pop[S ~[]E, E any](s S, i int) (S, E) {
	switch i {
	case 0:
		return s[1:], s[0]
	case len(s) - 1:
		return s[:len(s)-1], s[len(s)-1]
	}
	return append(s[:i], s[i+1:]...), s[i]
}

// Remove removes the elements s[i:j] from s, returning the modified slice
// and the removed elements. Remove panics if s[i:j] is not a valid slice of s.
func Remove[S ~[]E, E any](s S, i, j int) (S, S) {
	var s2 S
	if n := j - i; cap(s) >= len(s)+n {
		// Use the free space to avoid an allocation.
		s2 = s[len(s) : len(s)+n]
	} else {
		s2 = make(S, n)
	}
	copy(s2, s[i:j])
	return append(s[:i], s[j:]...), s2
}

@carlmjohnson
Copy link
Contributor

carlmjohnson commented Apr 20, 2022

Python has dict.pop which works like this. I don't think this behavior really makes sense for a slice though. It's not a very substantial savings versus just doing:

e := s[i]
s = slices.Delete(s, i, i+1)

It saves you a line, but it adds noise to the docs and open questions about behavior, when really we should be discouraging using O(N) operations more than necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

6 participants