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

s/PatKind::Ident/PatKind::Binding/g #970

Merged
merged 7 commits into from
May 31, 2016
Merged

s/PatKind::Ident/PatKind::Binding/g #970

merged 7 commits into from
May 31, 2016

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented May 31, 2016

This failed the most recent build.

@Manishearth
Copy link
Member

Still fails Travis. Also, version+changelog bump?

@llogiq
Copy link
Contributor Author

llogiq commented May 31, 2016

First it has to work. Something changed beyond the name...

@mcarton
Copy link
Member

mcarton commented May 31, 2016

You missed one.
Rust PR for reference: rust-lang/rust#33929.

@llogiq
Copy link
Contributor Author

llogiq commented May 31, 2016

The copies lint appears to find more matches now, which is nice. I'm a bit stumped on the matches test, though. @mcarton any idea?

@mcarton
Copy link
Member

mcarton commented May 31, 2016

This can warn:

    let _ = match Some(42) {
        Some(_) => 24,
        None => 24, //~ERROR this `match` has identical arm bodies
    };

It looks like None was a PatKind::Ident before and now is PatKind::Path, yayh smarter HIR 🎈

This should not:

    let _ = match Some(42) {
        Some(foo) => 24,
        None => 24,
    };

So you might want to add this in the test.

It also looks like you might need to rebase your local copy, there is still one PatKind::Ident left (see Travis on the PR).

@mcarton
Copy link
Member

mcarton commented May 31, 2016

For the match test you might need hir::PatKind::Path instead (we’re matching None &co here).

@@ -145,7 +145,7 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
(&PatKind::TupleStruct(ref lp, ref la, ls), &PatKind::TupleStruct(ref rp, ref ra, rs)) => {
self.eq_path(lp, rp) && over(la, ra, |l, r| self.eq_pat(l, r)) && ls == rs
}
(&PatKind::Ident(ref lb, ref li, ref lp), &PatKind::Ident(ref rb, ref ri, ref rp)) => {
Copy link
Member

Choose a reason for hiding this comment

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

You might also want to add PatKind::Path here and add some tests for it. Something like:

if let None = Some(42) {
} else if let None = Some(42) {
}

See rust-lang/rust#31685.

@llogiq
Copy link
Contributor Author

llogiq commented May 31, 2016

Now on to matches...

@@ -200,7 +200,8 @@ fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr:
}
path.to_string()
}
PatKind::Ident(BindByValue(MutImmutable), ident, None) => ident.node.to_string(),
PatKind::Binding(BindByValue(MutImmutable), ident, None) => ident.node.to_string(),
PatKind::Path(ref path) => path.to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Is Binding really necessary here? My understanding was that variant were now resolved (and we only care about some known paths).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, but I'm tired now and have to get up early tomorrow.

@mcarton
Copy link
Member

mcarton commented May 31, 2016

Looks like you failed your rebase/merge, some commits from master have appeared in the PR with a different hash.

@llogiq
Copy link
Contributor Author

llogiq commented May 31, 2016

I guess we can squash on merging. 😉

@mcarton mcarton merged commit 5f0f64c into master May 31, 2016
@mcarton mcarton deleted the rustup branch May 31, 2016 21:54
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