fix stack overflow by enum and const by mikhail-m1 · Pull Request #36458 · rust-lang/rust

mikhail-m1 · 2016-09-13T20:10:45Z

rust-highfive · 2016-09-13T20:10:53Z

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

mikhail-m1 · 2016-09-14T18:27:38Z

I'll fix it

bors · 2016-09-22T05:45:04Z

☔ The latest upstream changes (presumably #36551) made this pull request unmergeable. Please resolve the merge conflicts.

mikhail-m1 · 2016-09-22T18:30:26Z

@arielb1 Could you look at my PR or may be suggest other reviewer? I'll fix conflicts ASAP

arielb1

Your PR looks great, however:

There are a few issues with the coding style. You should fix them.
Your PR comment should mention the issue fixed - that some paths were skipped while checking for recursion.

arielb1 · 2016-09-24T09:27:10Z

Why the 'b? Lifetime elision should insert it by itself.

arielb1 · 2016-09-24T09:28:48Z

Why 2 lifetimes here? What was the problem with only 'a?

arielb1 · 2016-09-24T09:30:01Z

Should be a NodeSet and not a Vec, to avoid O(n^2).

nvm this is only non-empty when errors occur. Might want to add a comment to that effect?

arielb1 · 2016-09-24T09:35:18Z

nit: indentation.

nit: function should be named process_def or something.

arielb1 · 2016-09-24T09:37:42Z

nit: indentation. rustc style is to break after the equals sign. are you using rustfmt? Then don't. rustfmt sucks.

mikhail-m1 · 2016-10-04T19:56:22Z

@arielb1 Than you for review and sorry for long delay and rustfmt, I forgot run it after last fix.
I've fixed all nits, except two lifetime ('a and 'b). One need for session, maps, .. and another for detected_recursive_ids field from CheckCrateVisitor. Current version of CheckItemRecursionVisitor get descriminant_map as RefCell but I think two life time is better thand RefCell. May be I miss something. I am going to remove RefCell from descriminant_map too, but should it be separate PR?

bors · 2016-10-05T02:55:50Z

☔ The latest upstream changes (presumably #36814) made this pull request unmergeable. Please resolve the merge conflicts.

… were skipped while checking for recursion.

arielb1 · 2016-10-05T17:57:35Z

@bors r+

bors · 2016-10-05T17:57:35Z

📌 Commit 60891c9 has been approved by arielb1

bors · 2016-10-05T19:36:53Z

⌛ Testing commit 60891c9 with merge 6784ba3...

fix stack overflow by enum and const fixes#36163

bors · 2016-10-05T21:08:06Z

💔 Test failed - auto-win-msvc-64-cargotest

arielb1 · 2016-10-05T21:50:38Z

Timeout

@bors retry

bors · 2016-10-05T22:28:06Z

⚡ Previous build results for auto-mac-cross-ios-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild...

alexcrichton · 2016-10-05T23:20:17Z

@bors: retry force clean

looks like a new builder was forgotten by accident now

bors · 2016-10-05T23:20:19Z

⌛ Testing commit 60891c9 with merge 4a492ff...

bors · 2016-10-06T01:10:33Z

💔 Test failed - auto-win-msvc-64-cargotest

alexcrichton · 2016-10-06T01:56:00Z

@bors: retry

On Wednesday, October 5, 2016, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-cargotest
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/2004
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36458 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95Ab5dWc9RkSQgy0cR8m_dgYFYSUWks5qxEqMgaJpZM4J8F00
.

bors · 2016-10-06T04:25:34Z

⌛ Testing commit 60891c9 with merge 5102bde...

bors · 2016-10-06T05:43:11Z

💔 Test failed - auto-win-msvc-64-cargotest

alexcrichton · 2016-10-06T16:42:38Z

@bors: retry

On Wed, Oct 5, 2016 at 10:43 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-cargotest
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/2006
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36458 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95PPe22eVUXsKECTT4lEwIINQ8cuhks5qxIpzgaJpZM4J8F00
.

alexcrichton · 2016-10-06T17:22:31Z

@bors: r-

er wait, this seems legitimate!

mikhail-m1 · 2016-10-06T20:32:50Z

@alexcrichton How can I reproduce it locally?

alexcrichton · 2016-10-06T20:53:00Z

@mikhail-m1

I believe this should work:

./configure --enable-rustbuild
make check-cargotest

mikhail-m1 · 2016-10-07T21:15:01Z

@alexcrichton I rebuild it locally after pull and rebase and all tests passed successfully but on x86_64-unknown-linux-gnu

mikhail-m1 · 2016-10-11T11:19:17Z

What should be my next step?

alexcrichton · 2016-10-11T14:50:44Z

How long did the build take? It looks like something is becoming perhaps pathologically slow although it may still complete (e.g. the build failed as it lacked output for awhile)

mikhail-m1 · 2016-10-11T15:53:45Z

@alexcrichton step with url crate (I think it's cargotest) took several seconds, may be some way to get some information from CI build? I can rebuild it again collect and share output.

mikhail-m1 · 2016-10-14T20:59:19Z

testing https://github.com/iron/iron
...
Finished debug [unoptimized + debuginfo] target(s) in 258.97 secs
testing https://github.com/rust-lang/cargo
...
Finished debug [unoptimized + debuginfo] target(s) in 743.97 secs
but some tests failed, but my PR cannot affect it

full log: https://www.dropbox.com/s/wbi7esu7a9mapxx/build2.log?dl=0

alexcrichton · 2016-10-31T20:43:41Z

Closing due to inactivity but feel free to resubmit with tests fixed!

@alexcrichton

fix stack overflow by enum and cont issue #36163 some paths were skipped while checking for recursion. I fixed bug reproduces on win64 cargo test. In previous PR #36458 time complexity was exponential in case of linked const values. Now it's linear. r? @alexcrichton

rust-highfive assigned arielb1 Sep 13, 2016

mikhail-m1 force-pushed the stack-overflow branch from bd5872e to 4972dd5Compare September 13, 2016 21:01

mikhail-m1 force-pushed the stack-overflow branch 2 times, most recently from ab3bb8a to dc4a0cbCompare September 16, 2016 16:22

mikhail-m1 changed the title ~~fix stack overflow by enum and cont~~fix stack overflow by enum and constSep 16, 2016

arielb1 suggested changes Sep 24, 2016
View reviewed changes

mikhail-m1 force-pushed the stack-overflow branch from dc4a0cb to d04e264Compare October 4, 2016 19:46

fix stack overflow by enum and cont issuerust-lang#36163, some paths…
60891c9
… were skipped while checking for recursion.

mikhail-m1 force-pushed the stack-overflow branch from d04e264 to 60891c9Compare October 5, 2016 06:55

bors added a commit that referenced this pull request Oct 5, 2016
Auto merge of#36458- mikhail-m1:stack-overflow, r=arielb1
6784ba3
fix stack overflow by enum and const fixes#36163

alexcrichton closed this Oct 31, 2016

mikhail-m1 mentioned this pull request Nov 30, 2016
fix stack overflow by enum and cont issue #36163 #38093
Merged

Uh oh!

Uh oh!

Conversation

mikhail-m1 commented Sep 13, 2016

Uh oh!

rust-highfive commented Sep 13, 2016

Uh oh!

mikhail-m1 commented Sep 14, 2016

Uh oh!

bors commented Sep 22, 2016

Uh oh!

mikhail-m1 commented Sep 22, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1 left a comment

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016

Choose a reason for hiding this comment

Uh oh!

arielb1Sep 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhail-m1 commented Oct 4, 2016

Uh oh!

bors commented Oct 5, 2016

Uh oh!

arielb1 commented Oct 5, 2016

Uh oh!

bors commented Oct 5, 2016

Uh oh!

bors commented Oct 5, 2016

Uh oh!

bors commented Oct 5, 2016

Uh oh!

arielb1 commented Oct 5, 2016

Uh oh!

bors commented Oct 5, 2016

Uh oh!

alexcrichton commented Oct 5, 2016

Uh oh!

bors commented Oct 5, 2016

Uh oh!

bors commented Oct 6, 2016

Uh oh!

alexcrichton commented Oct 6, 2016

Uh oh!

bors commented Oct 6, 2016

Uh oh!

bors commented Oct 6, 2016

Uh oh!

alexcrichton commented Oct 6, 2016

Uh oh!

alexcrichton commented Oct 6, 2016

Uh oh!

mikhail-m1 commented Oct 6, 2016

Uh oh!

alexcrichton commented Oct 6, 2016

Uh oh!

mikhail-m1 commented Oct 7, 2016

Uh oh!

mikhail-m1 commented Sep 22, 2016•
edited
Loading

arielb1Sep 24, 2016•
edited
Loading

arielb1Sep 24, 2016•
edited
Loading

arielb1Sep 24, 2016•
edited
Loading

arielb1Sep 24, 2016•
edited
Loading

mikhail-m1 commented Oct 11, 2016•
edited
Loading