Skip to content

Demuting libsyntax #11167

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

Closed
wants to merge 73 commits into from
Closed

Demuting libsyntax #11167

wants to merge 73 commits into from

Conversation

pcwalton
Copy link
Contributor

No description provided.

end(&mut s); // Close the outer box
eof(&mut s.s);

// XXX(pcwalton): Need checked downcasts.
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little disturbing. There are 3 instances of this instead of 1, and I'm curious why you forget the writer because it looks like you moved out of s and now you own it?

This seems like a serious problem, and it seems like everything should be expecting &mut Writer instead of ~Writer in order to fix this. Do you know if it's possible to make the transition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't move out of s.

I could add lifetime parameters everywhere but I'm not sure if it's possible and it would make the code more ugly. DSTs will fix this (making it possible to cast from any ~Trait to ~Any, where you can downcast as you like) so I'm kind of reluctant to spend a lot of time on something that DSTs will fix. There is also the possibility of fixing this by allowing casts from ~Trait to any trait that is universally implemented (such as ~Any) in the frontend.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least this needs consolidation into having the unsafety in one location instead of 3. Can you open an issue about making this use &mut Writer instead of ~Writer?

Copy link
Member

Choose a reason for hiding this comment

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

r=me with the consolidation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, like I said I'd rather keep the ~Writer and fix the problem in the language, because I think we need the ability to convert ~Trait to ~Any for downcasting: this is going to keep biting us.

bors added a commit that referenced this pull request Dec 30, 2013
* Pass `&ExtCtxt` instead of `@ExtCtxt`.
* Stop passing duplicate parameters around in `expand`.
* Make `ast_fold` methods take `&mut self`.

After these, it should be possible to remove the `@mut` boxes from `ExtCtxt` altogether, though #11167 is doing some of that so I'm holding off on that for now. This will probably conflict with that PR, so I'm guessing that one will have to be rebased on top of the other.

r? @pcwalton
@eddyb eddyb mentioned this pull request Dec 30, 2013
@pcwalton
Copy link
Contributor Author

pcwalton commented Jan 1, 2014

r? @alexcrichton

In particular I'd like the tests to be looked at.

I did the consolidation you asked before.

@pcwalton pcwalton closed this Jan 4, 2014
@pcwalton pcwalton deleted the demuting branch January 4, 2014 07:49
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
…=Manishearth

Don't lint `needless_return` in fns across a macro boundary

Fixes rust-lang#11167

changelog: none
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