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

Cleanup ty decoder #27851

Merged
merged 7 commits into from
Aug 16, 2015
Merged

Cleanup ty decoder #27851

merged 7 commits into from
Aug 16, 2015

Conversation

nikomatsakis
Copy link
Contributor

Just a little code cleanup I was doing as part of another refactoring (which may turn out not to be needed). The main thrust of this is to cleanup the interface to tydecode.rs to be less ridiculously repetitive. I also purged the generic "def-id conversion" parameter in favor of a trait object, just to reduce code duplication a bit and make the signatures a bit less messy. I measured the bootstrapping time to build stage2 with these changes, it was identical. (But it'd be easy enough to restore the unboxed closure if we wanted it.)

@rust-highfive
Copy link
Collaborator

r? @jroesch

(rust_highfive has picked a reviewer for you, use r? to override)

@nikomatsakis
Copy link
Contributor Author

oh, one more commit coming.

@nikomatsakis
Copy link
Contributor Author

ok.

parse_ty_data(tp.data, cdata.cnum, tp.start, tcx,
|_, did| translate_def_id(cdata, did))
TyDecoder::with_doc(tcx, cdata.cnum, tp,
&mut |_, did| translate_def_id(cdata, did))
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this be created by a method on cdata, providing the translation function to TyDecoder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eddyb hmm perhaps -- it's a bit tricky to get lifetime of the closure to work out I think, but I guess now that the closure is stored in a struct, we could give ownership of it instead of passing an &mut. Would be kind of nicer anyway. Maybe I should do that.

@eddyb
Copy link
Member

eddyb commented Aug 16, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2015

📌 Commit 7a3a1be has been approved by eddyb

bors added a commit that referenced this pull request Aug 16, 2015
Just a little code cleanup I was doing as part of another refactoring (which may turn out not to be needed). The main thrust of this is to cleanup the interface to `tydecode.rs` to be less ridiculously repetitive. I also purged the generic "def-id conversion" parameter in favor of a trait object, just to reduce code duplication a bit and make the signatures a bit less messy. I measured the bootstrapping time to build stage2 with these changes, it was identical. (But it'd be easy enough to restore the unboxed closure if we wanted it.)
@bors
Copy link
Contributor

bors commented Aug 16, 2015

⌛ Testing commit 7a3a1be with merge fc7efab...

@bors bors merged commit 7a3a1be into rust-lang:master Aug 16, 2015
@bors bors mentioned this pull request Aug 16, 2015
5 tasks
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