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

Closed
thatradius opened this issue Apr 19, 2022 · 14 comments
Closed

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

thatradius opened this issue Apr 19, 2022 · 14 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

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

@ianlancetaylor Like Delete but returns the removed element.

@thatradius
Copy link
Author

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

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

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

@DeedleFake
Copy link

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

@thatradius
Copy link
Author

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
}

@earthboundkid
Copy link
Contributor

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.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

Pop should not be an O(N) operation. Saving s[i] and calling Delete seems fine for this use case. Delete is a bit problematic already; better not to have two.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@nightlyone
Copy link
Contributor

Pop could also return a book whether the element was found.

Then the panic on empty slice would be handled in a way that is consistent with e.g. the map check

Adding more ways to panic in Go doesn't sound particularly Go like to me, as Go programs are supposed to handle such conditions explicitly instead of checking preconditions and react on failure to do so.

Slice access is one of the notable exceptions.

@earthboundkid
Copy link
Contributor

Go programs are supposed to handle such conditions explicitly instead of checking preconditions and react on failure to do so.

I agree "Go programs are supposed to handle such conditions explicitly" but I don't see how it follows that being explicit is "instead of checking preconditions". For map, if you couldn't do _, ok := m[k] how would you explicitly check that the map has a key? Comma ok is checking the precondition. For a slice, the equivalent check is if len(s) > i. There doesn't need to be comma ok version of slice access because we have if len(s) > i.

Anyway, Pop is O(N) and really should not be encouraged.

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Apr 12, 2023
@golang golang locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants