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

Allow casting to mutable trait objects. #5725

Merged
merged 1 commit into from
Apr 18, 2013
Merged

Conversation

jdm
Copy link
Contributor

@jdm jdm commented Apr 4, 2013

No description provided.

@jdm
Copy link
Contributor Author

jdm commented Apr 4, 2013

r? @pcwalton

@nikomatsakis
Copy link
Contributor

@jdm -- I don't think this is quite the right way to handle the representation etc in the compiler

@jdm
Copy link
Contributor Author

jdm commented Apr 7, 2013

I appreciate the feedback, but would you like to be a bit more specific? It's a bit hard to act on your comment. If this is covered by #5762, that's fine, but I would like to know what the next step is.

@nikomatsakis
Copy link
Contributor

Yes, sorry! That was brief. Let me expand. First off, I apologize, when I first read your patch I was somewhat mislead because you used the letters mt to refer to the mutability, but mt is usually the pair of a mutability and a type. Including an mt in ty_trait wouldn't make much sense. It would probably be better to just use m or mutbl if the value is just a mutability.

Anyhow, adding a mutability to ty_trait doesn't strike me as ideal, since it doesn't apply e.g. to ~Trait, but it's also not a bad thing. We have other cases in our type representation that are "more general" than they need to be. I tend to think we should refactor those, and perhaps we can eventually update this along the lines of #5762, but in the meantime its ok.

I haven't had time to read over the patch in great detail to tell if you're covering all the necessary checks yet though, but it's a good idea to get the basic syntax etc in place.

@jdm
Copy link
Contributor Author

jdm commented Apr 14, 2013

Getting the syntax in Rust is blocking my work on Servo. I still can't tell whether you're just asking for some renaming for clarity, or whether you're going to need to find some time to read through it more deeply before giving more directed feedback.

@brson
Copy link
Contributor

brson commented Apr 15, 2013

@nikomatsakis Any further feedback on this?

@nikomatsakis
Copy link
Contributor

@jdm @brson sorry, in short, my feeling is that the patch is basically ok as is, particularly if this is a blocking issue. I think we'll wind up refactoring it some down the line but that goes for just about every patch.

@nikomatsakis
Copy link
Contributor

(though I would certainly prefer not using the mnemonic mt to refer to just a mutability, since that is typically a ty::mt.)

@jdm
Copy link
Contributor Author

jdm commented Apr 16, 2013

I'll rebase and rename the mts.

@jdm
Copy link
Contributor Author

jdm commented Apr 17, 2013

Rebased and renamed, and much more pleasant to read after Niko's trait changes! r? @nikomatsakis

bors added a commit that referenced this pull request Apr 18, 2013
@bors bors closed this Apr 18, 2013
@bors bors merged commit 9730370 into rust-lang:incoming Apr 18, 2013
@jdm jdm deleted the muttrait branch April 18, 2013 08:19
flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 28, 2020
should_impl_trait - ignore methods with lifetime params

Fixes: rust-lang#5617

changelog: don't lint should_implement_trait when an `Iterator::next` case has explicit parameters
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.

4 participants