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

Fix cast-related ICEs #24158

Merged
merged 4 commits into from Apr 9, 2015

Conversation

Projects
None yet
7 participants
@sanxiyn
Copy link
Member

sanxiyn commented Apr 7, 2015

Fix #13993.
Fix #17167.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 7, 2015

r? @Aatch

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 7, 2015

cc @nrc

@nrc

This comment has been minimized.

Copy link

nrc commented on src/librustc_typeck/check/cast.rs in f4c2228 Apr 7, 2015

nit: put the !( on the same line as the clause

@nrc

This comment has been minimized.

Copy link

nrc commented on src/librustc_typeck/check/mod.rs in d18b405 Apr 7, 2015

need to check ty_uniq too

This comment has been minimized.

Copy link

nrc replied Apr 7, 2015

in fact, better to use ty::deref than to do the manual match

This comment has been minimized.

Copy link
Owner Author

sanxiyn replied Apr 8, 2015

I am not sure about using ty::deref, as it returns an Option as well as mutability which we don't need here.

This comment has been minimized.

Copy link

nrc replied Apr 8, 2015

The option part is pretty simple - if it returns None, then its not a pointer, so you can return false. You do throw away mutability, but you save code dup - you wouldn't have missed ty_uniq if you'd used it here and if we add another pointer type in the future, we might miss this function.

This comment has been minimized.

Copy link
Owner Author

sanxiyn replied Apr 8, 2015

That's convincing. Done.

@nrc

This comment has been minimized.

Copy link

nrc commented on src/test/compile-fail/fat-ptr-cast.rs in d18b405 Apr 7, 2015

should make each cast a separate statement so that we make sure the error is on the correct cast. Also, please add tests for the & and Box cases

@nrc nrc assigned nrc and unassigned Aatch Apr 7, 2015

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 7, 2015

lgtm, r=me with the comments addressed

@sanxiyn

This comment has been minimized.

Copy link
Member Author

sanxiyn commented Apr 8, 2015

Addressed review comments.

@sanxiyn sanxiyn force-pushed the sanxiyn:cast branch from a1b6923 to e2ff188 Apr 8, 2015

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Apr 8, 2015

I would also add a test for cast from a ?Sized type parameter:

fn nullptr<T: ?Sized>() -> *const T {
    0 as *const _
}
fn main() {
    nullptr::<[();3]>();
    nullptr::<[()]>();
}

Also, &_ as usize is a non-scalar cast anyway.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Apr 8, 2015

There are more interesting cases:

use std::fmt;
use std::iter::IntoIterator;

fn ptr_cast<U: ?Sized, V: ?Sized>(u: *const U) -> *const V
{
   u as *const _
}

fn main() {
    let i0 = [5u32];
    let i1 = i0.into_iter();
    let i2 : &Iterator<Item=&u32> = &i1;
    let i3 = i2 as *const Iterator<Item=&u32>;
    let p: *const fmt::Display = ptr_cast::<_, fmt::Display>(i3);
}

I guess this needs a separate issue.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 8, 2015

bors added a commit that referenced this pull request Apr 9, 2015

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 9, 2015

⌛️ Testing commit e2ff188 with merge 287a544...

@bors bors merged commit e2ff188 into rust-lang:master Apr 9, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@sanxiyn sanxiyn deleted the sanxiyn:cast branch Apr 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.