Skip to content

minor refactorings around InferCtxt::enter #49020

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

nikomatsakis
Copy link
Contributor

Just a few small simplifications.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2018
@bors
Copy link
Collaborator

bors commented Mar 15, 2018

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

@nikomatsakis nikomatsakis force-pushed the chalkify-cleanup-enter-sig branch from 8432c77 to 84252f6 Compare March 15, 2018 10:34
@@ -189,7 +189,7 @@ impl<'a, 'tcx> MaybeInProgressTables<'a, 'tcx> {
/// `bar()` will each have their own `FnCtxt`, but they will
/// share the inherited fields.
pub struct Inherited<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
infcx: InferCtxt<'a, 'gcx, 'tcx>,
infcx: &'a InferCtxt<'a, 'gcx, 'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

Why the extra indirection?

pub fn enter<F, R>(&'tcx mut self, f: F) -> R
where F: for<'b> FnOnce(InferCtxt<'b, 'gcx, 'tcx>) -> R
{
pub fn enter<'tcx, R>(&'tcx mut self, f: impl FnOnce(&InferCtxt<'_, 'gcx, 'tcx>) -> R) -> R {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it started here - why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert that part if you like -- I was actually working towards a goal with this patch series that didn't work out, so it's not really important either way (I just figured I'd land some of the incidental changes).

That said, I always get annoyed that enter gives me ownership of an InferCtxt, when almost everything wants a reference. I'd be quite surprised if the indrection around Inherited cost performance.

@eddyb
Copy link
Member

eddyb commented Mar 16, 2018

r=me with travis failure fixed

@pietroalbini pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2018
@bors
Copy link
Collaborator

bors commented Mar 22, 2018

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

@nikomatsakis
Copy link
Contributor Author

Sigh. I should really rebase this.

@shepmaster
Copy link
Member

Ping from triage, @nikomatsakis ! We agree you should rebase this ❤️. Might as well respond to the review feedback you got while you are at it!

@nikomatsakis
Copy link
Contributor Author

Eh. I don't care enough to rebase this. =)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants