Skip to content

Refactor: {Lvalue,Rvalue,Operand}::ty only need the locals' types, not the full &Mir #43174

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

Merged
merged 4 commits into from
Jul 15, 2017

Conversation

RalfJung
Copy link
Member

I am writing code that needs to call these ty methods while mutating MIR -- which is impossible with the current API.

Even with the refactoring the situation is not great: I am cloning the local_decls and then passing the clone to the ty methods. I have to clone because Mir::basic_blocks_mut borrows the entire Mir including the local_decls. But even that is better than not being able to get these types at all...

Cc @nikomatsakis

@rust-highfive
Copy link
Contributor

r? @eddyb

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

@RalfJung
Copy link
Member Author

RalfJung commented Jul 12, 2017

A way to avoid the cloning may be to wrap basic_blocks and cache into their own type; then Mir::basic_blocks_mut would only have to borrow these two bits.

The refactoring in this PR however would still be needed.

EDIT: I saw the tidy errors, but I'm going to wait for feedback if this approach will be accepted at all before fixing them.

@arielb1
Copy link
Contributor

arielb1 commented Jul 12, 2017

You can allow passing the MIR to .ty using AsRef<LocalDecls> to make the code cleaner:

impl<'tcx> AsRef<LocalDecls<'tcx>> for LocalDecls<'tcx> { /* .. */ }
impl<'tcx> AsRef<LocalDecls<'tcx>> for Mir<'tcx> { /* .. */ }

    fn ty<'a, 'gcx, D: AsRef<LocalDecls>>(&self, local_decls: D, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> LvalueTy<'tcx> {
        self.ty_(local_decls.as_ref(), tcx)
    }

    fn ty_<'a, 'gcx>(&self, local_decls: &LocalDecls<'tcx>, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> LvalueTy<'tcx> {
        // ..
    }

@arielb1
Copy link
Contributor

arielb1 commented Jul 12, 2017

A way to avoid the cloning may be to wrap basic_blocks and cache into their own type; then Mir::basic_blocks_mut would only have to borrow these two bits.

The cloning does not look like such big of a deal. We need to figure out a "builder" interface for MIR, but that's for the future.

@RalfJung
Copy link
Member Author

You can allow passing the MIR to .ty using AsRef to make the code cleaner:

For this I have to wrap LocalDelcs into a newtype, or else coherence checks kick in.

Would that be all right, or too hevay-weight?

@nikomatsakis
Copy link
Contributor

@RalfJung in which crate are you adding the impls?

@RalfJung
Copy link
Member Author

@nikomatsakis in librustc.

@RalfJung
Copy link
Member Author

I made a new trait for this overloading.

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2017
@RalfJung
Copy link
Member Author

Should I be annotating these fn local_decls with #[inline]? Seems silly for them to ever not be inlined.

@RalfJung
Copy link
Member Author

Oh, and another thought: I could monomorphize the big functions taking HasLocalDecls (change them to something private that takes &LocalDelcs), and only make small shims polymorphic. That reduces code size. Is it worth it though?

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 14, 2017

📌 Commit 0bbc315 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Jul 14, 2017

⌛ Testing commit 0bbc315 with merge 23ecebd...

bors added a commit that referenced this pull request Jul 14, 2017
Refactor: {Lvalue,Rvalue,Operand}::ty only need the locals' types, not the full &Mir

I am writing code that needs to call these `ty` methods while mutating MIR -- which is impossible with the current API.

Even with the refactoring the situation is not great: I am cloning the `local_decls` and then passing the clone to the `ty` methods. I have to clone because `Mir::basic_blocks_mut` borrows the entire `Mir` including the `local_decls`. But even that is better than not being able to get these types at all...

Cc @nikomatsakis
@bors
Copy link
Collaborator

bors commented Jul 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 23ecebd to master...

@bors bors merged commit 0bbc315 into rust-lang:master Jul 15, 2017
@RalfJung RalfJung deleted the refactor-ty branch July 15, 2017 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants