Merged
Conversation
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
Contributor
nikomatsakis
requested changes
Dec 17, 2018
Contributor
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Looks like a good first step! Left a nit.
src/librustc_typeck/check/upvar.rs
Outdated
| }; | ||
|
|
||
| self.tcx.with_freevars(closure_node_id, |freevars| { | ||
| let mut freevar_list: Vec<ty::UpvarId> = Vec::new(); |
Contributor
There was a problem hiding this comment.
Nit: I would use Vec::with_capacity -- or, perhaps even better, convert to an iterator:
let upvar_ids: Vec<_> = freevars.iter().map(|freevar| ty::UpvarId {..}).collect();
self.tables
.borrow_mut()
.upvar_list
.insert(closure_def_id, upvar_ids);
for upvar_id in &upvar_ids {
... // as before
}
Contributor
Author
There was a problem hiding this comment.
I agree with the with_capacity part to save on the vector reallocation but by using freevars.iter().map(...).collect() etc. we are iterating over the freevars twice (once here and other time in for freevar in freevars {.. ). Wouldn't you say that's less performant ?
Contributor
There was a problem hiding this comment.
Potentially. It seems unlikely to matter in practice, so I was more concerned with what "read better".
You could of course do both at once by adding a inspect in the chain:
let upvar_ids: Vec<_> = freevars.iter()
.map(|freevar| ty::UpvarId {..})
.inspect(|&upvar_id| {
let capture_kind = /* as before */;
self.tables.borrow_mut().insert(upvar_id, capture_kind);
})
.collect();I tend to prefer not to use explicit with_capacity calls and instead use iterators/collector, but either one is fine. Matter of personal preference I suppose.
Contributor
|
@bors r+ rollup |
Collaborator
|
📌 Commit d751954 has been approved by |
bors
added
S-waiting-on-bors
emilyalbini
added a commit
to emilyalbini/rust
that referenced
this pull request
Dec 20, 2018
Issue rust-lang#56905 Adding a map to TypeckTables to get the list of all the Upvars given a closureID. This is help us get rid of the recurring pattern in the codebase of iterating over the free vars using with_freevars.
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Dec 21, 2018
Issue rust-lang#56905 Adding a map to TypeckTables to get the list of all the Upvars given a closureID. This is help us get rid of the recurring pattern in the codebase of iterating over the free vars using with_freevars.
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Centril
added
S-waiting-on-author
src/librustc_mir/build/mod.rs
Outdated
Contributor
There was a problem hiding this comment.
@blitzerr the simplest fix is to invoke tcx.hir().hir_to_node_id =)
blitzerr
commented
Dec 29, 2018
src/librustc_mir/build/mod.rs
Outdated
Contributor
Author
There was a problem hiding this comment.
I am getting ICE because of this code that I have added.
thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0', /Users/blitz/rustc-dev/rust/src/libcore/slice/mod.rs:2455:10
lines 650 -657 are debug statements to compare the freevars as given by the with_freevars function vs the new map upvar_list we added(though the Freevar struct is very different from the UpvarId, we are just looking for presence of values and count). I guess the problem is that upvar_list contains no vec of upvars for the given DefId, while the with_freevars does.
When I added similar debug messages to upvar.rs, I could see values in the upvar_list but not in this file.
Contributor
Author
There was a problem hiding this comment.
from upvar.rs debug printfs
upvar.rs freevars: [Freevar { def: Local(NodeId(177103)), span: src/libstd/sys/unix/process/process_unix.rs:177:36: 177:38 }]
upvar.rs upvars: [UpvarId(HirId { owner: DefIndex(0:6823), local_id: 33 };`fd`;DefId(0/1:2738 ~ std[e3a0]::sys[0]::unix[0]::process[0]::process_inner[0]::{{impl}}[0]::do_exec[0]::{{closure}}[0]))]
upvar.rs freevars: [Freevar { def: Local(NodeId(177113)), span: src/libstd/sys/unix/process/process_unix.rs:180:36: 180:38 }]
upvar.rs upvars: [UpvarId(HirId { owner: DefIndex(0:6823), local_id: 73 };`fd`;DefId(0/1:2739 ~ std[e3a0]::sys[0]::unix[0]::process[0]::process_inner[0]::{{impl}}[0]::do_exec[0]::{{closure}}[1]))]
upvar.rs freevars: [Freevar { def: Local(NodeId(177123)), span: src/libstd/sys/unix/process/process_unix.rs:183:36: 183:38 }]
upvar.rs upvars: [UpvarId(HirId { owner: DefIndex(0:6823), local_id: 113 };`fd`;DefId(0/1:2740 ~ std[e3a0]::sys[0]::unix[0]::process[0]::process_inner[0]::{{impl}}[0]::do_exec[0]::{{closure}}[2]))]
upvar.rs freevars: [Freevar { def: Local(NodeId(177874)), span: src/libstd/sys/unix/process/process_unix.rs:415:41: 415:45 }, Freevar { def: Local(NodeId(177903)), span: src/libstd/sys/unix/process/process_unix.rs:415:56: 415:62 }]
upvar.rs upvars: [UpvarId(HirId { owner: DefIndex(0:6845), local_id: 87 };`self`;DefId(0/1:2688 ~ std[e3a0]::sys[0]::unix[0]::process[0]::process_inner[0]::{{impl}}[1]::wait[0]::{{closure}}[0])), UpvarId(HirId { owner: DefIndex(0:6845), local_id: 21 };`status`;DefId(0/1:2688 ~ std[e3a0]::sys[0]::unix[0]::process[0]::process_inner[0]::{{impl}}[1]::wait[0]::{{closure}}[0]))]
===================================================================================
Old upvars: [Freevar { def: Local(NodeId(177103)), span: src/libstd/sys/unix/process/process_unix.rs:177:36: 177:38 }]
Old upvars: [Freevar { def: Local(NodeId(177113)), span: src/libstd/sys/unix/process/process_unix.rs:180:36: 180:38 }]
Old upvars: [Freevar { def: Local(NodeId(177123)), span: src/libstd/sys/unix/process/process_unix.rs:183:36: 183:38 }]
Old upvars: [Freevar { def: Local(NodeId(177874)), span: src/libstd/sys/unix/process/process_unix.rs:415:41: 415:45 }, Freevar { def: Local(NodeId(177903)), span: src/libstd/sys/unix/process/process_unix.rs:415:56: 415:62 }]
Contributor
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
requested changes
Jan 8, 2019
Contributor
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Looking good! Left some minor nits, but I think we can merge this.
src/librustc/ty/context.rs
Outdated
src/librustc_mir/build/mod.rs
Outdated
Contributor
There was a problem hiding this comment.
Nit: I suggest you rewrite this like so:
let upvar_decls: Vec<_> = hir_tables.upvar_list.get(&fn_def_id).iter().flatten().map(|upvar_id| { ... }).collect();instead of using a match. Here, the iter is iterating over the option (and then using flatten to process the upvar lists). Just a bit less indentation.
Contributor
Author
There was a problem hiding this comment.
will do and re-post
Contributor
There was a problem hiding this comment.
Nit: use //, not ///, as this is not a doc comment
Contributor
There was a problem hiding this comment.
Also, i'd prefer a blank line
// Entry point
//
// During type inference, ...
Or perhaps move this to a module doc comment (i.e, using //!)?
Contributor
Author
There was a problem hiding this comment.
Sure, will move it to module doc comment.
Adding a map to TypeckTables to get the list of all the Upvars given a closureID. This is help us get rid of the recurring pattern in the codebase of iterating over the free vars using with_freevars.
Change the key of UpvarListMap from DefId to ast::NodeId
nikomatsakis
approved these changes
Jan 9, 2019
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit 69e4918 has been approved by |
bors
added
S-waiting-on-bors
Collaborator
|
⌛ Testing commit 69e4918 with merge 16912382d40270ae0d302acd6aafa247bf7f405a... |
Collaborator
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
Member
|
@bors retry |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jan 12, 2019
Issue rust-lang#56905 Adding a map to TypeckTables to get the list of all the Upvars given a closureID. This is help us get rid of the recurring pattern in the codebase of iterating over the free vars using with_freevars.
bors
added a commit
that referenced
this pull request
Jan 12, 2019
Rollup of 26 pull requests Successful merges: - #56425 (Redo the docs for Vec::set_len) - #56906 (Issue #56905) - #57042 (Don't call `FieldPlacement::count` when count is too large) - #57175 (Stabilize `let` bindings and destructuring in constants and const fn) - #57192 (Change std::error::Error trait documentation to talk about `source` instead of `cause`) - #57296 (Fixed the link to the ? operator) - #57368 (Use CMAKE_{C,CXX}_COMPILER_LAUNCHER for ccache) - #57400 (Rustdoc: update Source Serif Pro and replace Heuristica italic) - #57417 (rustdoc: use text-based doctest parsing if a macro is wrapping main) - #57433 (Add link destination for `read-ownership`) - #57434 (Remove `CrateNum::Invalid`.) - #57441 (Supporting backtrace for x86_64-fortanix-unknown-sgx.) - #57450 (actually take a slice in this example) - #57459 (Reference tracking issue for inherent associated types in diagnostic) - #57463 (docs: Fix some 'second-edition' links) - #57466 (Remove outdated comment) - #57493 (use structured suggestion when casting a reference) - #57498 (make note of one more normalization that Paths do) - #57499 (note that FromStr does not work for borrowed types) - #57505 (Remove submodule step from README) - #57510 (Add a profiles section to the manifest) - #57511 (Fix undefined behavior) - #57519 (Correct RELEASES.md for 1.32.0) - #57522 (don't unwrap unexpected tokens in `format!`) - #57530 (Fixing a typographical error.) - #57535 (Stabilise irrefutable if-let and while-let patterns) Failed merges: r? @ghost
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding a map to TypeckTables to get the list of all the Upvars
given a closureID. This is help us get rid of the recurring
pattern in the codebase of iterating over the free vars
using with_freevars.
#56905