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

core: Add Default::replace_default(&mut self) #33564

Closed
wants to merge 1 commit into from

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented May 11, 2016

No description provided.

@cuviper
Copy link
Member Author

cuviper commented May 11, 2016

@bluss wanted something like take() for Vec. A few folks liked my bikeshed name.
ref: https://internals.rust-lang.org/t/method-take-on-vec/3371/13

@samlh
Copy link

samlh commented May 11, 2016

I would prefer the bikeshed to be painted replace_with_default, as I was really confused by the title of the PR :)

@cuviper
Copy link
Member Author

cuviper commented May 11, 2016

Hmm, replace_with_default is getting wordy, but I can see how that's clearer. Any other ideas? I will yield to popular opinion.

For an analogy, this is a bit like C++ move semantics, where the value is transferred but the origin is still left in a usable (unspecified) state. Here we're specifying the source will be default().

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 11, 2016
@alexcrichton alexcrichton self-assigned this May 11, 2016
@alexcrichton
Copy link
Member

Thanks for the PR! This is a stable prelude trait so despite this method being unstable it's borderline "requires an RFC" territory. (I could kinda go either way, cc @rust-lang/libs)

Also, could you elaborate on some of the rationale for this as well? Can this lead to optimizations in some cases or is it just a more ergonomic mem::replace?

@cuviper
Copy link
Member Author

cuviper commented May 12, 2016

Yes, it's mostly just ergonomic, a generalization of Option::take. You could also optimize for classes that don't need a full memcpy, like the uninitialized portions of an ArrayVec. But that's probably already paying a full memcpy even for simple moves, so this might make little difference.

@alexcrichton
Copy link
Member

Hm ok, if it's purely ergonomic then I'd definitely recommend an RFC :)

@cuviper
Copy link
Member Author

cuviper commented May 12, 2016

Dang, I thought I had an easy one... :)

@bluss any thoughts? Does this meet your need?

@bluss
Copy link
Member

bluss commented May 12, 2016

Thanks for writing the PR.

I don't think it is an easy one. I don't prefer changing Default, but I can see that it has some advantages.^

The original suggestion was to add a method .take() on Vec which does the same as this method (and the same thing that Option::take does too).

The inspiration for that is threefold, the similarity with Option::take, the regular need for this call, and thirdly to make it easy to read and explain the code. It's one of those inspirations that come from discussing solutions to simple situations with newcomers to rust.

It does functionally meet the need (but the functionality was already present with replace), it is ergonomic, so that's a great plus, but with this name I don't think it is as nice as .take(), so it does not really fill my need of how I want to express myself in rust. I realize a method on a widely used trait like Default can not be named take, especially not due to the conflict with HashSet::take.

I think this is an area where we should say to be correct: it can be implemented similarly in an external crate, and we should use experience from that before including.

^ Default advantage: baseline feature generally available
^ Default drawbacks: Requires long name because it's in the default name space, Is not guaranteed to be cheap (A Take trait could possibly be specified to always be cheap to call, does not allocate a new replacement).

@cuviper
Copy link
Member Author

cuviper commented May 13, 2016

OK, thanks for all the feedback!

To be honest, I was only mildly interested in this, enough to try for the low-hanging fruit. If someone else would like to go further with an RFC and/or external crate, please feel free!

@cuviper cuviper closed this May 13, 2016
@cuviper cuviper deleted the replace_default branch April 3, 2020 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants