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

Fix invalid trait generation #33935

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #33919.

r? @steveklabnik

And a screenshot to come along:

screenshot from 2016-05-29 01 37 40

@eefriedman
Copy link
Contributor

fn eq(&self, other: RefCell<T>) -> bool
fn ne(&self, other: RefCell) -> bool

That doesn't look quite right.

@GuillaumeGomez
Copy link
Member Author

Oh indeed. Going back to add missing information then.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented May 29, 2016

New screenshot (lifetimes and type parameters are now generated as well):

screenshot from 2016-05-29 23-43-46

@GuillaumeGomez
Copy link
Member Author

A more complete screenshot to see lifetimes as well:

untitled

@steveklabnik
Copy link
Member

I think conceptually, I like this change, but I am gonna review the code more a bit later. If anyone else is happy with the code, feel free to r+ it.

@steveklabnik
Copy link
Member

This seems okay to me, but I am still getting to know rustdoc. @rust-lang/tools do any of you have thoughts on this patch?

@alexcrichton
Copy link
Member

The issue this is fixing, #33919, is mostly just a dupe of #14072 which is a general problem that rustdoc doesn't substitute type parameters, so it just shows up this way. In general I think this may be a pretty ad-hoc solution which doesn't actually get us much farther.

It looks like this only handles Self, right? And it also only handles &Self and and Self as well? That is, I don't think cases like Box<Self> seem to be handled.

I think this would be best done as a preparation pass during rustdoc, either during the "clean" phase or as a dedicated pass in rustdoc itself. The rendered is already pretty complicated with lots of functions taking lots of arguments, so offloading as much as we can elsewhere seems beneficial.

@GuillaumeGomez
Copy link
Member Author

@alexcrichton: That's what I tried at first but got quickly stuck with my lack of knowledge on the "low" code. I'll try to redo it in the "preparation pass".

@bors
Copy link
Contributor

bors commented Jul 1, 2016

☔ The latest upstream changes (presumably #34541) made this pull request unmergeable. Please resolve the merge conflicts.

@steveklabnik
Copy link
Member

@GuillaumeGomez are you still working on this?

@steveklabnik
Copy link
Member

Closing due to a lack of activity, let me know if you want to keep up with this!

@GuillaumeGomez
Copy link
Member Author

Ah damn, didn't see the first comment. I'll, but don't know when.

@GuillaumeGomez GuillaumeGomez deleted the trait_doc_gen branch November 24, 2017 20:56
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