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

Implement slow-path for FirstSets::first #62956

Merged
merged 1 commit into from
Jul 26, 2019
Merged

Conversation

ia0
Copy link
Contributor

@ia0 ia0 commented Jul 24, 2019

When 2 or more sequences share the same span, we can't use the precomputed map
for their first set. So we compute it recursively.

Fixes #62831.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2019
@ia0
Copy link
Contributor Author

ia0 commented Jul 24, 2019

I'm not sure how to test that. Unit-test? But it fixes the ICE in #62831.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

nits

src/libsyntax/ext/tt/macro_rules.rs Outdated Show resolved Hide resolved
src/libsyntax/ext/tt/macro_rules.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

@ia0

I'm not sure how to test that.

Is it possible to extract some minimized reproduction from #62831?
A small procedural macro that generates one macro_rules item with two repetitions with the same span?

Otherwise, the fix LGTM.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2019
When 2 or more sequences share the same span, we can't use the precomputed map
for their first set. So we compute it recursively.

Fixes rust-lang#62831.
@ia0
Copy link
Contributor Author

ia0 commented Jul 25, 2019

I added a test that gets an ICE before the fix and fails as expected (so the test passes) after the fix.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2019

📌 Commit df4b23e has been approved by petrochenkov

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
Implement slow-path for FirstSets::first

When 2 or more sequences share the same span, we can't use the precomputed map
for their first set. So we compute it recursively.

Fixes rust-lang#62831.
bors added a commit that referenced this pull request Jul 26, 2019
Rollup of 14 pull requests

Successful merges:

 - #62084 (allow clippy::unreadable_literal in unicode tables)
 - #62421 (Introduce `as_deref` to Option)
 - #62692 (rustc: precompute the largest Niche and store it in LayoutDetails.)
 - #62801 (Remove support for -Zlower-128bit-ops)
 - #62828 (Remove vector fadd/fmul reduction workarounds)
 - #62862 (code cleanup)
 - #62897 (Attempt to fix backtrace tests on i686-msvc)
 - #62904 (Disable d32 on armv6 hf targets)
 - #62907 (Initialize the MSP430 AsmParser)
 - #62956 (Implement slow-path for FirstSets::first)
 - #62963 (Allow lexer to recover from some homoglyphs)
 - #62970 (ci: gate toolstate repo pushes on the TOOLSTATE_PUBLISH envvar)
 - #62983 (Remove needless indirection through Rc)
 - #62985 (librustc_errors: Support ui-testing flag in annotate-snippet emitter)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
Implement slow-path for FirstSets::first

When 2 or more sequences share the same span, we can't use the precomputed map
for their first set. So we compute it recursively.

Fixes rust-lang#62831.
Centril added a commit to Centril/rust that referenced this pull request Jul 26, 2019
Implement slow-path for FirstSets::first

When 2 or more sequences share the same span, we can't use the precomputed map
for their first set. So we compute it recursively.

Fixes rust-lang#62831.
bors added a commit that referenced this pull request Jul 26, 2019
Rollup of 22 pull requests

Successful merges:

 - #62084 (allow clippy::unreadable_literal in unicode tables)
 - #62120 (Add missing type links in documentation)
 - #62310 (Add missing doc links in boxed module)
 - #62421 (Introduce `as_deref` to Option)
 - #62583 (Implement Unpin for all raw pointers)
 - #62692 (rustc: precompute the largest Niche and store it in LayoutDetails.)
 - #62801 (Remove support for -Zlower-128bit-ops)
 - #62828 (Remove vector fadd/fmul reduction workarounds)
 - #62862 (code cleanup)
 - #62904 (Disable d32 on armv6 hf targets)
 - #62907 (Initialize the MSP430 AsmParser)
 - #62956 (Implement slow-path for FirstSets::first)
 - #62963 (Allow lexer to recover from some homoglyphs)
 - #62964 (clarify and unify some type test names)
 - #62970 (ci: gate toolstate repo pushes on the TOOLSTATE_PUBLISH envvar)
 - #62980 (std: Add more accessors for `Metadata` on Windows)
 - #62983 (Remove needless indirection through Rc)
 - #62985 (librustc_errors: Support ui-testing flag in annotate-snippet emitter)
 - #63002 (error_index_generator should output stdout/stderr when it panics.)
 - #63004 (Add test for issue-54062)
 - #63007 (ci: debug network failures while downloading awscli from PyPI)
 - #63009 (Remove redundant `mut` from variable declaration.)

Failed merges:

r? @ghost
@bors bors merged commit df4b23e into rust-lang:master Jul 26, 2019
@ia0 ia0 deleted the fix_62831 branch July 27, 2019 09:48
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.

assume all sequences have (unique) spans for now
5 participants