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

Remove the default Send bound on traits #13050

Merged
merged 3 commits into from Mar 27, 2014

Conversation

alexcrichton
Copy link
Member

See #10296 for the rationale, and commits for the implementation.

@flaper87
Copy link
Contributor

LGTM. I always try to keep each commit "compilable" so, I'd prefer if these commits were squashed.

@huonw
Copy link
Member

huonw commented Mar 21, 2014

@flaper87 these cases are often a good time to break that (useful) rule, to make it super-clear what the core change is, and what the purely mechanical fix-ups are.

@flaper87
Copy link
Contributor

@huonw agreed, I find them useful as well. Perhaps those could be squashed after the review and before the final approve. With that we'd loose the benefit of these commits being split in the history, though. Anyway, probably not a big deal.

@huonw
Copy link
Member

huonw commented Mar 22, 2014

There doesn't seem to be a way to specific bounds on generic trait objects (#9265). Neither of these parse:

trait Foo<T> {}

let _: ~Foo:Freeze<int>;
let _: ~Foo<int>:Freeze;

This PR is blocked on that issue.

@alexcrichton
Copy link
Member Author

Why is this blocked on that issue? The issue is certainly more pressing, but it would be nice to get off the default bounds as soon as possible.

@huonw
Copy link
Member

huonw commented Mar 22, 2014

Because without the new parsing there's literally no way to have a sendable generic trait object; while the current situation just means you're forced to use sendable ones even when a non-sendable one is sufficient. (In any case, the point is moot since you opened #13079.)

@nikomatsakis
Copy link
Contributor

👍 I'm happy to see this, I was just thinking "Oh, I should go implement that by now, might be something I can do in my spare time." Instead, I'll review. :)

@@ -686,7 +686,7 @@ fn test_repr() {
exact_test(&println, "fn(&str)");
exact_test(&swap::<int>, "fn(&mut int, &mut int)");
exact_test(&is_alphabetic, "fn(char) -> bool");
exact_test(&(~5 as ~ToStr), "~to_str::ToStr:Send");
exact_test(&(~5 as ~ToStr), "~to_str::ToStr<no-bounds>");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have got to change this printout. (not necessarily in this PR)

@nikomatsakis
Copy link
Contributor

The changes look good to me. I say r+ pending resolution of @huonw's objection.

@alexcrichton
Copy link
Member Author

@nikomatsakis, @huonw, with #13079 soon-to-land (hopefully!), are you guys ok with this?

@nikomatsakis, you may also be interested in #13155 as to why I had to write :Send so often when I was hoping to just write a supertrait bound on all the Rtio traits.

@flaper87
Copy link
Contributor

@alexcrichton could you also update the docs? There's a line in the tutorial that says ~Trait:Send is equivalent to ~Trait

@alexcrichton
Copy link
Member Author

@flaper87, good catch, I've updated the docs a bit.

@huonw
Copy link
Member

huonw commented Mar 27, 2014

r=me with minor docs edits

This commit removes implicitly adding the Send bound to ~Trait objects and
procedure types. It will now be manually required to specify that a procedure
or trait must be send-able.

Closes rust-lang#10296
This is all purely fallout of getting the previous commit to compile.
bors added a commit that referenced this pull request Mar 27, 2014
See #10296 for the rationale, and commits for the implementation.
@bors bors closed this Mar 27, 2014
@bors bors merged commit 8d0be73 into rust-lang:master Mar 27, 2014
@alexcrichton alexcrichton deleted the no-send-default branch March 28, 2014 07:14
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.

None yet

5 participants