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

Simplify irrefutable slice patterns #47374

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Conversation

topecongiro
Copy link
Contributor

Closes #47096.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@eddyb
Copy link
Member

eddyb commented Jan 12, 2018

r? @nikomatsakis

@topecongiro
Copy link
Contributor Author

Fixed test failures & rebased.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 12, 2018
PatternKind::Slice { ref prefix, ref slice, ref suffix } => {
if prefix.is_empty() && slice.is_some() && suffix.is_empty() {
// irrefutable
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems missing here. Doesn't this produce a binding for the slice? If so, don't we have to extend candidate.bindings?

Maybe try an example that actually uses the binding? e.g.

let [foo..] = s;
println!("{:?}", foo);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for you review! I tried the example, and it died with segmentation fault. So yes, we need to extend candidate.bindings. I will update this PR to do so.

@topecongiro
Copy link
Contributor Author

Update this PR and rebased.

#![allow(dead_code)]

fn foo(s: &[i32]) {
let &[ref _xs..] = s;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this test to actually use xs?

For example:

fn foo(s: &[i32]) -> &[i32] {
    let &[ref s1..] = s;
    s1
}

fn main() {
    let x = [1, 2, 3];
    let y = foo(&x);
    assert_eq!(y, &[1, 2, 3]);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I forgot to do this.

// irrefutable
candidate
.match_pairs
.push(MatchPair::new(match_pair.place, slice.as_ref().unwrap()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, so, something here makes me a bit nervous. Why not invoke the helper prefix_slice_suffix? For example:

self.prefix_slice_suffix(
    &mut candidate.match_pairs,
    match_pair.place,
    prefix,
    slice,
    suffix,
);

that helper is more general than strictly needed, but I think it is sure to do the right thing, or at least we can do the wrong thing all in the same code. =)

Copy link
Contributor

Choose a reason for hiding this comment

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

(In this particular case, the main difference would be that the Place would include a Subslice projection, I think, which may not be needed, but I still think i'd rather go through the helper.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In prefix_slice_suffix(), we have

        if let Some(subslice_pat) = opt_slice {
            let subslice = place.clone().elem(ProjectionElem::Subslice {
                from: prefix.len() as u32,
                to: suffix.len() as u32
            });
            match_pairs.push(MatchPair::new(subslice, subslice_pat));
        }

so ProjectionElem::Subslice.to is set to suffix.len() which is 0 in this case.

Doc comment on Subslice in librustc/mir/mod.rs says:

    /// These indices are generated by slice patterns.
    ///
    /// slice[from:-to] in Python terms.
    Subslice {
        from: u32,
        to: u32,
    },

In Python, slice[from:-0] will be an empty list. This is why I avoided using prefix_slice_suffix().

However, it turns that setting to to 0 is actually equivalent to slice[from:] in Python term. So I agree that using prefix_slice_suffix() is better here.

@nikomatsakis
Copy link
Contributor

@bors r+

I'm assuming travis will go green here =)

@bors
Copy link
Contributor

bors commented Jan 17, 2018

📌 Commit 604a54f has been approved by nikomatsakis

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2018
@bors
Copy link
Contributor

bors commented Jan 24, 2018

⌛ Testing commit 604a54f with merge 65ffb2deb981432d5f5405b530ed479655fbaae0...

@bors
Copy link
Contributor

bors commented Jan 24, 2018

💥 Test timed out

@kennytm
Copy link
Member

kennytm commented Jan 24, 2018

@bors retry

4 hour timeout due to the asm.js job restarting.

@bors
Copy link
Contributor

bors commented Jan 25, 2018

⌛ Testing commit 604a54f with merge 247835a...

bors added a commit that referenced this pull request Jan 25, 2018
Simplify irrefutable slice patterns

Closes #47096.
@bors
Copy link
Contributor

bors commented Jan 25, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 247835a to master...

@bors bors merged commit 604a54f into rust-lang:master Jan 25, 2018
@topecongiro topecongiro deleted the issue-47096 branch January 25, 2018 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants