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: builtin: delete returns bool #41130

Closed
pjebs opened this issue Aug 29, 2020 · 6 comments
Closed

proposal: builtin: delete returns bool #41130

pjebs opened this issue Aug 29, 2020 · 6 comments

Comments

@pjebs
Copy link
Contributor

@pjebs pjebs commented Aug 29, 2020

It would be good if the delete function returns a bool indicating if the key was found or not. if it was found, it returns true and the key is obviously deleted.

In my article: https://medium.com/swlh/ordered-maps-for-go-using-generics-875ef3816c71#4102, you can see that after I attempt to delete a key from the map, I have to do an expensive operation (optimising the data struct using linked list is besides the point). The operation only needs to be done if the key exists.

I can't check existence for key first and then attempt to delete because it's not atomic.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Aug 29, 2020

You mention check-then-delete is not atomic, which indicates you want them for concurrent use, but maps are not safe for concurrent use without synchronization, so the correct solution would be to introduce a lock or some other method of synchronization which would also cover check-then-delete

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Aug 29, 2020

Using a synchronisation mechanism is overkill in my situation. I just need to be informed if the delete() actually deleted a key or not. Internally, the function has that information. It just needs to release it.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Aug 29, 2020

On second thoughts, you are correct but I still think this proposal provides benefits with no cost and is backward compatible

@ianlancetaylor ianlancetaylor changed the title builtin: delete returns bool (atomic) proposal: builtin: delete returns bool (atomic) Aug 30, 2020
@gopherbot gopherbot added this to the Proposal milestone Aug 30, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 30, 2020
@rsc rsc changed the title proposal: builtin: delete returns bool (atomic) proposal: builtin: delete returns bool Sep 2, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

In the non-atomic case, this boils down to:

if _, ok := m[key]; ok {
    delete(m, key) // you know key was deleted
}

This seems like pretty clear code.
The only real argument against it is that it might be inefficient to do the hash twice.
We could fix that in the compiler (#5147) if needed.

It doesn't seem worth a language change.

But you wrote:

I can't check existence for key first and then attempt to delete because it's not atomic.

If there are other map changes happening at the same time, your code is unsafe and will crash, either from the race detector or from the map implementation's own race detector. Adding an 'atomic' delete would require synchronizing every write and delete just in case there was a racing write, which would slow down all accesses. That's why maps aren't atomic in the first place.

Given that maps aren't atomic and you need an atomic operation, I think it's safe to say this is infeasible.

@rsc rsc moved this from Incoming to Active in Proposals Sep 2, 2020
@mewmew

This comment has been hidden.

@pjebs pjebs closed this Sep 2, 2020
@rsc rsc moved this from Active to Declined in Proposals Sep 16, 2020
@golang golang locked and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants