syntax: Rewrite parsing of patterns by petrochenkov · Pull Request #23930 · rust-lang/rust

petrochenkov · 2015-04-01T02:49:23Z

Fixes#22757
Fixes#22972
Fixes#23044
Fixes#23151
Fixes#23597
Fixes#23656
Fixes#23929
It also fixes some other corner cases in range patterns, like incorrect spans or not accepting global paths after ....

It passes make check but needs some additional tests (then it will fix#22546 as well), I'll write them today or tomorrow.

rust-highfive · 2015-04-01T02:49:36Z

r? @nrc

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

alexcrichton · 2015-04-01T17:04:10Z

Nice!

nrc · 2015-04-02T02:44:11Z

Could you explain the reasoning behind why it is safe to remove this check please?

Flag RESTRICTION_NO_BAR_OP is checked here but it isn't set anywhere, so I removed the check and the flag itself. RESTRICTION_NO_BAR_OP was used before in parse_pat, see my line comments there.

nrc · 2015-04-02T02:54:00Z

lgtm, I just want to be clear about removing that one check

petrochenkov · 2015-04-02T07:01:43Z

Here parse_expr_res(RESTRICTION_NO_BAR_OP) was mistakenly used to parse plain identifier.

nrc · 2015-04-02T21:33:52Z

Ok, sounds good, thanks for the explanation.

@bors: r+ d9a0ca01f3938efbb34587e3f52214d8c0bc1624

bors · 2015-04-02T22:31:23Z

⌛ Testing commit d9a0ca0 with merge eb9ca18...

bors · 2015-04-02T22:56:27Z

💔 Test failed - auto-linux-64-x-android-t

petrochenkov · 2015-04-03T09:14:43Z

Rebased and fixed.

nrc · 2015-04-03T09:29:35Z

@bors: r+ 76567a6

bors · 2015-04-03T12:05:38Z

⌛ Testing commit 76567a6 with merge 0640106...

bors · 2015-04-03T12:59:09Z

⛄ The build was interrupted to prioritize another pull request.

bors · 2015-04-03T15:44:25Z

⌛ Testing commit 76567a6 with merge 80def6c...

Fixes#22757Fixes#22972Fixes#23044Fixes#23151Fixes#23597Fixes#23656Fixes#23929 It also fixes some other corner cases in range patterns, like incorrect spans or not accepting global paths after `...`. It passes `make check` but needs some additional tests (then it will fix#22546 as well), I'll write them today or tomorrow.

Fixesrust-lang#22757Fixesrust-lang#22972Fixesrust-lang#23044Fixesrust-lang#23151Fixesrust-lang#23597Fixesrust-lang#23656Fixesrust-lang#23929 It also fixes some other corner cases in range patterns, like incorrect spans or not accepting global paths after `...`. It passes `make check` but needs some additional tests (then it will fixrust-lang#22546 as well), I'll write them today or tomorrow.

bors · 2015-04-03T18:28:37Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

- Successful merges: #23930, #23941, #23972, #23976, #23978, #23993, #23995, #23997, #24005 - Failed merges:

ghost · 2015-04-03T18:54:55Z

@petrochenkov Did you mean to say that you'd add the test cases in a separate PR? As it is, none of the fixed issues have any related tests so perhaps we should reopen them?

petrochenkov · 2015-04-03T19:11:32Z

@jakub-
I meant to write the tests as a part of this PR but was busy, so I'm going to submit them separately. All the mentioned issues are essentially duplicates, so I'll open one blanket E-needstest issue for them if I don't submit the PR with tests in the next couple of hours.

This was supposed to be a part of rust-lang#23930

bombless · 2015-04-04T12:54:58Z

Hmm @bors closed #22546 after merging this, but I believe it is an irrelevant issue, since the core idea of that issue is about discussing whether we should allow type parameters in pattern.

bombless · 2015-04-04T12:56:03Z

cc @pnkfelix

brson · 2015-04-07T15:22:21Z

@bombless I reopened that issue for you.

brson · 2015-04-07T15:23:41Z

Awesome list of fixes.

pnkfelix · 2015-04-07T15:58:05Z

@bombless thanks for the pro-active note. However, during triage (I think), the team decided as noted in e.g. this comment that we would keep the syntax, and that we just needed to exercise the edge cases more thoroughly.

pnkfelix · 2015-04-07T16:00:40Z

But, skimming this PR, it seems like it did not actually add any tests relevant to #22546, so I guess that issue should remain open.

petrochenkov · 2015-04-07T16:09:08Z

@pnkfelix
The follow-up PR (#24033) contains some tests for #22546. I don't know, whether that enough or not.

rust-highfive assigned nrc Apr 1, 2015

nrc reviewed Apr 2, 2015
View reviewed changes

petrochenkov reviewed Apr 2, 2015
View reviewed changes

petrochenkov added 2 commits April 2, 2015 13:34

syntax: Rewrite parsing of patterns
35c1bdb

Fix parsing of patterns in macros
76567a6

petrochenkov force-pushed the issue23656 branch from d9a0ca0 to 76567a6Compare April 3, 2015 09:12

Manishearth mentioned this pull request Apr 3, 2015
Rollup of PRs in the queue; Saturday #24009
Merged

bors added a commit that referenced this pull request Apr 3, 2015
Auto merge of#24009- Manishearth:rollup, r=Manishearth
f77cb26
- Successful merges: #23930, #23941, #23972, #23976, #23978, #23993, #23995, #23997, #24005 - Failed merges:

bors merged commit 76567a6 into rust-lang:masterApr 3, 2015

bors mentioned this pull request Apr 3, 2015
Return nonzero exit code if there are errors at a stop point #22117
Merged

petrochenkov mentioned this pull request Apr 3, 2015
Add tests for parsing of patterns #24033
Merged

Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 4, 2015
Rollup merge ofrust-lang#24033- petrochenkov:pattest, r=alexcrichton
90b9b0d
This was supposed to be a part of rust-lang#23930

petrochenkov mentioned this pull request Apr 4, 2015
compiler panic on "let v[0] = v[1]" where v is a vector #23212
Closed

brson mentioned this pull request Apr 7, 2015
add more tests for type-instantiation pat::<T> syntax in patterns #22546
Closed

petrochenkov deleted the issue23656 branch May 9, 2015 11:57

Uh oh!

Uh oh!

Conversation

petrochenkov commented Apr 1, 2015

Uh oh!

rust-highfive commented Apr 1, 2015

Uh oh!

alexcrichton commented Apr 1, 2015

Uh oh!

nrcApr 2, 2015

Choose a reason for hiding this comment

Uh oh!

petrochenkovApr 2, 2015

Choose a reason for hiding this comment

Uh oh!

nrc commented Apr 2, 2015

Uh oh!

petrochenkovApr 2, 2015

Choose a reason for hiding this comment

Uh oh!

nrc commented Apr 2, 2015

Uh oh!

bors commented Apr 2, 2015

Uh oh!

bors commented Apr 2, 2015

Uh oh!

petrochenkov commented Apr 3, 2015

Uh oh!

nrc commented Apr 3, 2015

Uh oh!

bors commented Apr 3, 2015

Uh oh!

bors commented Apr 3, 2015

Uh oh!

bors commented Apr 3, 2015

Uh oh!

bors commented Apr 3, 2015

Uh oh!

ghost commented Apr 3, 2015

Uh oh!

petrochenkov commented Apr 3, 2015

Uh oh!

bombless commented Apr 4, 2015

Uh oh!

bombless commented Apr 4, 2015

Uh oh!

brson commented Apr 7, 2015

Uh oh!

brson commented Apr 7, 2015

Uh oh!

pnkfelix commented Apr 7, 2015

Uh oh!

pnkfelix commented Apr 7, 2015

Uh oh!

petrochenkov commented Apr 7, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants