Skip to content

Conversation

@dwrensha
Copy link
Contributor

@dwrensha dwrensha commented Jan 6, 2018

This should fix rust-lang/miri#360. I haven't actually tested it.

I believe this fix is correct because a similar fix works for Seer: dwrensha/seer@a071e1c

cc @oli-obk

@rust-highfive
Copy link
Contributor

r? @michaelwoerister

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

@oli-obk
Copy link
Contributor

oli-obk commented Jan 8, 2018

I don't think this'll work for ((),(),((), &i32)) or similarly nested things.

@michaelwoerister
Copy link
Member

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Jan 8, 2018

IMO this should "just" do untupling by doing what arg0 = tuple.0; arg1 = tuple.1; ... would do.
Which means using project_field or w/e and avoid all of this broken makeshift logic.

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 9, 2018
@kennytm
Copy link
Member

kennytm commented Jan 17, 2018

Hi @dwrensha, just to check if you're still working on this PR 😊

Could you apply @eddyb's suggestion in #47239 (comment)?

@dwrensha
Copy link
Contributor Author

I've updated Seer with a somewhat simplified version of this, including the testcase @oli-obk was concerned about: dwrensha/seer@f2962ea.

Unfortunately, the https://github.com/solson/miri testsuite is not currently runnable at rustc master (see rust-lang/miri#361), so it is difficult to verify whether changes like this are correct. I'll defer to @oli-obk and @eddyb on what to do with this pull request. My intent was mainly to bring attention to the problem.

@eddyb
Copy link
Member

eddyb commented Jan 20, 2018

My point was that the entire match args[1].value { can become a single loop using place_field.

@kennytm
Copy link
Member

kennytm commented Jan 31, 2018

Hi @dwrensha, checking again. Would you like to apply the suggested change in #47239 (comment), i.e. to refactor “the entire match into a single loop using place_field”? Or should we just close it since the intention was “mainly to bring attention to the problem”?

Also cc @oli-obk, as OP mentioned in #47239 (comment), it seems difficult to move this PR forward without miri testing re-enabled, which in turn is blocked by #46882.

@pietroalbini
Copy link
Member

@dwrensha and @oli-obk, ping from triage!

@oli-obk
Copy link
Contributor

oli-obk commented Feb 5, 2018

Let's close this until I get the miri submodule working again. Until then it's not testable and additionally this PR is not the preferred way to solve this

@pietroalbini
Copy link
Member

Ok, closing this. @dwrensha, thank you for this PR anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

8 participants