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

Enable env communication #894

Merged
merged 12 commits into from
Aug 14, 2019
Merged

Enable env communication #894

merged 12 commits into from
Aug 14, 2019

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Aug 6, 2019

related issue: #800. r? @RalfJung

src/eval.rs Show resolved Hide resolved
src/eval.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2019

Please also add a corresponding bool to Evaluator and initialize it with this boolean, so that the flag is available during execution.

src/eval.rs Outdated Show resolved Hide resolved
@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 6, 2019

Should I still keep the field in Evaluator? I could populate the hashmap with the env variables without having to store the communicate boolean.

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2019

Would you like to add this to the README as well?

Should I still keep the field in Evaluator?

Yes. Future extensions of this system might need it at run-time.
This PR is more generally useful than just the env var PR it paves the ground for. :)

@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 6, 2019

I decided to keep going. I'm adding some comments in certain parts where I'd like to have some feedback because I'm not feeling too sure about them.

@pvdrz pvdrz changed the title Add flag to enable env communication Enable env communication Aug 6, 2019
src/eval.rs Outdated Show resolved Hide resolved
src/shims/env.rs Show resolved Hide resolved
tests/run-pass/communication.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2019

I decided to keep going.

I asked you to keep it separate to make review easier. :/ I would have landed the other thing this morning. You could have kept going locally without updating this PR. Just have mutliple branches and then when the nits are done here rebase your other branch.

But I don't have time for a bigger review right now. Maybe on the weekend.

src/shims/env.rs Outdated Show resolved Hide resolved
src/shims/env.rs Show resolved Hide resolved
tests/run-pass/communication.rs Outdated Show resolved Hide resolved
src/shims/env.rs Show resolved Hide resolved
@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 7, 2019

I decided to keep going.

I asked you to keep it separate to make review easier. :/ I would have landed the other thing this morning. You could have kept going locally without updating this PR. Just have mutliple branches and then when the nits are done here rebase your other branch.

But I don't have time for a bigger review right now. Maybe on the weekend.

Sorry :/ I did not think about it. Anyway we have no rush here I guess.

tests/compiletest.rs Outdated Show resolved Hide resolved
src/eval.rs Outdated Show resolved Hide resolved
src/eval.rs Show resolved Hide resolved
src/eval.rs Outdated Show resolved Hide resolved
@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 13, 2019

I believe we're ready to go

src/eval.rs Outdated Show resolved Hide resolved
src/eval.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/eval.rs Outdated Show resolved Hide resolved
src/eval.rs Show resolved Hide resolved
src/shims/env.rs Outdated Show resolved Hide resolved
src/eval.rs Show resolved Hide resolved
src/shims/env.rs Outdated Show resolved Hide resolved
src/eval.rs Show resolved Hide resolved
@RalfJung
Copy link
Member

Thanks for your patience!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 14, 2019

📌 Commit 451a09a has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Aug 14, 2019

⌛ Testing commit 451a09a with merge 1f504ea...

bors added a commit that referenced this pull request Aug 14, 2019
@pvdrz
Copy link
Contributor Author

pvdrz commented Aug 14, 2019

I am going to start migrating the env related code in shims::foreign_items to shims::env::EnvVars when this is merged!

@bors
Copy link
Contributor

bors commented Aug 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 1f504ea to master...

@bors bors merged commit 451a09a into rust-lang:master Aug 14, 2019
@pvdrz pvdrz deleted the env-vars-communication branch August 14, 2019 19:23
@RalfJung
Copy link
Member

Uh, this is odd:

warning: field is never used: `communicate`
  --> src/machine.rs:97:5
   |
97 |     pub(crate) communicate: bool,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

impl EnvVars {
pub(crate) fn init<'mir, 'tcx>(
ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>,
communicate: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I entirely forgot that the machine has this flag. So can you remove this parameter, and use ecx.machine.communicate instead?

Copy link
Member

@RalfJung RalfJung Aug 15, 2019

Choose a reason for hiding this comment

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

Since you are probably asleep, I'll do this as part of #909.

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