fix stack overflow by enum and const #36458
Conversation
|
r? @arielb1 (rust_highfive has picked a reviewer for you, use r? to override) |
mikhail-m1
force-pushed
the
stack-overflow
branch
from
bd5872e
to
4972dd5
Compare
|
I'll fix it |
mikhail-m1
force-pushed
the
stack-overflow
branch
2 times, most recently
from
ab3bb8a
to
dc4a0cb
Compare
mikhail-m1
changed the title
|
☔ The latest upstream changes (presumably #36551) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@arielb1 Could you look at my PR or may be suggest other reviewer? I'll fix conflicts ASAP |
| } | ||
|
|
||
| impl<'a, 'ast: 'a> Visitor<'ast> for CheckCrateVisitor<'a, 'ast> { | ||
| fn visit_item(&mut self, it: &'ast hir::Item) { | ||
| fn visit_item<'b>(&'b mut self, it: &'ast hir::Item) { |
There was a problem hiding this comment.
Why the 'b? Lifetime elision should insert it by itself.
| sess: &'a Session, | ||
| ast_map: &'a ast_map::Map<'ast>, | ||
| def_map: &'a DefMap, | ||
| struct CheckItemRecursionVisitor<'a, 'b: 'a, 'ast: 'b> { |
There was a problem hiding this comment.
Why 2 lifetimes here? What was the problem with only 'a?
| @@ -34,10 +34,11 @@ struct CheckCrateVisitor<'a, 'ast: 'a> { | |||
| // each one. If the variant uses the default values (starting from `0`), | |||
| // then `None` is stored. | |||
| discriminant_map: RefCell<NodeMap<Option<&'ast hir::Expr>>>, | |||
| detected_ids: Vec<ast::NodeId>, | |||
There was a problem hiding this comment.
Should be a NodeSet and not a Vec, to avoid O(n^2).
There was a problem hiding this comment.
nvm this is only non-empty when errors occur. Might want to add a comment to that effect?
|
|
||
| fn process_node(&mut self, id: ast::NodeId, span: Span) { | ||
| match self.def_map.get(&id).map(|d| d.base_def) { | ||
| Some(Def::Static(def_id, _)) | |
There was a problem hiding this comment.
nit: function should be named process_def or something.
| // might be (if any). | ||
| Some(Def::Variant(enum_id, variant_id)) => { | ||
| if let Some(enum_node_id) = self.ast_map.as_local_node_id(enum_id) { | ||
| if let hir::ItemEnum(ref enum_def, ref generics) = self.ast_map |
There was a problem hiding this comment.
nit: indentation. rustc style is to break after the equals sign. are you using rustfmt? Then don't. rustfmt sucks.
mikhail-m1
force-pushed
the
stack-overflow
branch
from
dc4a0cb
to
d04e264
Compare
|
@arielb1 Than you for review and sorry for long delay and rustfmt, I forgot run it after last fix. |
|
☔ The latest upstream changes (presumably #36814) made this pull request unmergeable. Please resolve the merge conflicts. |
… were skipped while checking for recursion.
mikhail-m1
force-pushed
the
stack-overflow
branch
from
d04e264
to
60891c9
Compare
|
@bors r+ |
|
📌 Commit 60891c9 has been approved by |
fix stack overflow by enum and const fixes #36163
|
💔 Test failed - auto-win-msvc-64-cargotest |
@bors retry |
|
@bors: retry force clean
|
|
⌛ Testing commit 60891c9 with merge 4a492ff... |
|
💔 Test failed - auto-win-msvc-64-cargotest |
|
@bors: retry On Wednesday, October 5, 2016, bors notifications@github.com wrote:
|
|
⌛ Testing commit 60891c9 with merge 5102bde... |
|
💔 Test failed - auto-win-msvc-64-cargotest |
|
@bors: retry On Wed, Oct 5, 2016 at 10:43 PM, bors notifications@github.com wrote:
|
|
@bors: r- er wait, this seems legitimate! |
|
@alexcrichton How can I reproduce it locally? |
|
I believe this should work: ./configure --enable-rustbuild
make check-cargotest |
|
@alexcrichton I rebuild it locally after pull and rebase and all tests passed successfully but on x86_64-unknown-linux-gnu |
|
What should be my next step? |
|
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) |
|
@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. |
|
testing https://github.com/iron/iron full log: https://www.dropbox.com/s/wbi7esu7a9mapxx/build2.log?dl=0 |
|
Closing due to inactivity but feel free to resubmit with tests fixed! |
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
fixes #36163