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

Add a test for the new type_name intrinsic. #761

Merged
merged 1 commit into from
Jun 6, 2019

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