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

prohibit splitting literal patterns #35664

Merged
merged 1 commit into from
Aug 15, 2016

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Aug 14, 2016

Before PR #32202, check_match tended to report bogus errors or ICE
when encountering a pattern that split a literal, e.g.

match foo {
    "bar" => {},
    &_ => {}
}

That PR fixed these issues, but trans::_match generates bad code
when it encounters these matches. MIR trans has that fixed, but
it is waiting for 6 weeks in beta. Report an error when encountering
these instead.

Fixes issue #35044.

Before PR rust-lang#32202, check_match tended to report bogus errors or ICE
when encountering a pattern that split a literal, e.g.

```Rust
match foo {
    "bar" => {},
    &_ => {}
}
```

That PR fixed these issues, but trans::_match generates bad code
when it encounters these matches. MIR trans has that fixed, but
it is waiting for 6 weeks in beta. Report an error when encountering
these instead.

Fixes issue rust-lang#35044.
@arielb1
Copy link
Contributor Author

arielb1 commented Aug 14, 2016

cc @brson

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@alexcrichton
Copy link
Member

Hm we're cutting it really close for a fix like this, the stable build kicks off tomorrow. Are we pretty confident in this change? @arielb1 does this already exist on master and is being tested?

I'd be fine merging a fix but just want to make sure it warrants last-minute very-little-testing status.

@arielb1
Copy link
Contributor Author

arielb1 commented Aug 14, 2016

On master we use MIR, so we do not need the temporary restriction.

@eddyb
Copy link
Member

eddyb commented Aug 14, 2016

LGTM, @nikomatsakis what do you think?

@nikomatsakis
Copy link
Contributor

r+ from me

I agree with @alexcrichton that this is cutting it close, but this is a fairly simple change. Seems about as close to "risk free" as you can get.

@alexcrichton alexcrichton merged commit 9b21dcd into rust-lang:beta Aug 15, 2016
pmatos pushed a commit to LinkiTools/rust that referenced this pull request Sep 27, 2016
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.

6 participants