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

Refactor how we handle overflow and remove blanket Pattern impl #23580

Merged
merged 2 commits into from
Mar 24, 2015

Conversation

nikomatsakis
Copy link
Contributor

See comments in the commits for reasoning

r? @aturon
cc @Kimundi

@@ -474,22 +474,16 @@ impl<'a, 'b> Pattern<'a> for &'b [char] {
s, CharEqPattern(s));
}

/// Searches for chars that are equal to any of the chars in the array
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be the doc comment of the &str impl instead, or rather explaining that it delegates to the &str impl.

@Kimundi
Copy link
Member

Kimundi commented Mar 21, 2015

Apart from the doc comments, the pattern impls look fine to me.

@alexcrichton
Copy link
Member

Is it possible to still have a forwarding impl, but just for references?

impl<'a, 'b, P: Pattern<'a> + ?Sized + 'b> Pattern<'a> for &'b P {
    ...
}

@nikomatsakis
Copy link
Contributor Author

@alexcrichton no, because any type P: Fn is also P: Pattern, and if we make it so that &F is Fn, then you have two overlapping routes to derive that &P: Pattern.

@nikomatsakis nikomatsakis force-pushed the pattern-and-overflow branch from 9d68f33 to 8ff0d74 Compare March 23, 2015 20:40
@nikomatsakis
Copy link
Contributor Author

rebased, tweaked docs per @Kimundi's suggestion

@aturon
Copy link
Member

aturon commented Mar 23, 2015

@bors: r+ 8ff0d74

compilation: this removes all the ungainly code that special cases
overflow so that we can ensure it propagates.
possible blanket impls and also triggers internal overflow. Add some
special cases for common uses (&&str, &String) for now; bounds-targeting
deref coercions are probably the right longer term answer.
@nikomatsakis nikomatsakis force-pushed the pattern-and-overflow branch from 8ff0d74 to 76ead08 Compare March 23, 2015 22:05
@nikomatsakis
Copy link
Contributor Author

@bors r=aturon 7dead08

@bors
Copy link
Contributor

bors commented Mar 23, 2015

🙀 7dead08 is not a valid commit SHA. Please try again with 76ead08.

@nikomatsakis
Copy link
Contributor Author

@bors r=aturon 76ead08

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2015
@bors bors merged commit 76ead08 into rust-lang:master Mar 24, 2015
@ghost
Copy link

ghost commented Mar 26, 2015

I think the overflow refactoring here might be what @aturon was referring to in the issue I reported at #23714. The overflow/failure on evaluating the Sized bound causes my ackermann.rs crate to no longer compile since I am using Cow in a rose-tree like structure here which triggers the issue.

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.

5 participants