Skip to content

Conversation

@ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 3, 2019

Addresses this comment from @RalfJung.

This makes the type_name intrinsic call librustc_mir::interpret::type_name instead of Ty::to_string.

I used the implementation of eval_const_to_op as a reference.

This now adds a test for the type_name intrinsic and removes the miri impl in favor of the in-core one.

@ecstatic-morse ecstatic-morse changed the title Use the official implementation of type_name. Use the new implementation of type_name. Jun 3, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

I'd prefer to wait for rust-lang/rust#61399 to get merged and then remove the impl here

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2019

Okay. @ecstatic-morse could you reduce this PR to just the test case, and remove the intrinsic implementation? We can then merge that once the Rust PR lands.

@ecstatic-morse ecstatic-morse changed the title Use the new implementation of type_name. Add a test for the type_name intrinsic. Jun 5, 2019
@ecstatic-morse
Copy link
Contributor Author

@RalfJung Done!

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2019

Could you also remove the implementation of the intrinsic from src/intrinsic.rs? It should not be needed any more now that we have a rustc-side implementation. You'll have to update rust-version to point to the latest commit from upstream rustc though.

And please rebase onto current master.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2019

Oh actually never mind, rust-lang/rust#61498 did not land yet.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2019

Tests are failing though:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"std::option::Option<i32>"`,
 right: `"core::option::Option<i32>"`', $DIR/intrinsics.rs:14:5

@oli-obk
Copy link
Contributor

oli-obk commented Jun 5, 2019

They will keep failing without a bump of the rustc version we're using in CI

@ecstatic-morse ecstatic-morse changed the title Add a test for the type_name intrinsic. Add a test for the new type_name intrinsic. Jun 5, 2019
@ecstatic-morse
Copy link
Contributor Author

I'll retrigger a CI run when rust-lang/rust#61498 lands.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2019

You'll also have to update the rust-version file -- we are pinning the version that CI uses.

@ecstatic-morse
Copy link
Contributor Author

@RalfJung

I updated rust-version to the latest nightly and Travis is green.

We bump `rust-version` to pick up the new impl from
rust-lang/rust#61498 and add a test.
@oli-obk oli-obk merged commit 6ab0147 into rust-lang:master Jun 6, 2019
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.

3 participants