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

Make TaskBuilder methods return self to support Builder pattern. #8708

Closed

Conversation

pnkfelix
Copy link
Member

Fix #6399.

(At least in principle, though Niko has noted in the comments there that this might not accomplish everything one might have desired.)

@bblum
Copy link
Contributor

bblum commented Aug 23, 2013

Really they need to take self by value. They are only not doing that because they were written before we had value-self, and there is some code (called "consume") I think to "fake" it. Can you make this change too?

All the usage sites should be changed from the t.foo(); t.bar(); pattern to the t.foo().bar() pattern. Using &mut self won't catch these for you, and leaving them that way could result in some confusing bugs.

@pnkfelix
Copy link
Member Author

@bblum Maybe I misunderstood the goal here: Are we trying to encode a purely-functional Builder pattern (and thus it should indeed take self by value, and create copies), or the imperative one (and thus take &'a mut self, and return that linearly-passed object)?

@metajack might also have an opinion here.

Since I'm not the ciient for this code, I think I'm just going to drop my effort on it, since its not actually something I care about investing much effort in.

@pnkfelix
Copy link
Member Author

(closing the PR; someone else is free to clone my branch and land it or a variant of it if they want to act as its champion.)

@pnkfelix
Copy link
Member Author

(also, just to note: the reason that I believed that the goal here was an imperative builder was that I interpreted the changes of commit 803a4f4 as deliberately choosing to switch from a version that did make copies to one that used imperative update. However, its possible that that switch from copying-to-mutation was just a side-artifact, not a design goal in itself.)

@bblum
Copy link
Contributor

bblum commented Aug 24, 2013

I think that commit was just cleaning up in anticipation of having no mutable fields. The interface was making copies in the past, and invalidating the previous copies, to "fake" self-by-value, which it wanted to be all along.

flip1995 pushed a commit to flip1995/rust that referenced this pull request May 5, 2022
…endoo,xFrednet

fix ICE in `cast_slice_different_sizes`

fixes rust-lang#8708

changelog: fixes an ICE introduced in rust-lang#8445
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.

Make TaskBuilder methods return Self again so we can use the builder pattern.
2 participants