Issue #56905 #56906
Merged
Issue #56905 #56906
Conversation
This file contains 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
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
nikomatsakis
requested changes
Dec 17, 2018
src/librustc_typeck/check/upvar.rs
Outdated
| @@ -132,14 +132,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { | |||
| }; | |||
|
|
|||
| self.tcx.with_freevars(closure_node_id, |freevars| { | |||
| let mut freevar_list: Vec<ty::UpvarId> = Vec::new(); | |||
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
}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 ?
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.
|
@bors r+ rollup |
|
📌 Commit d751954 has been approved by |
bors
added
S-waiting-on-bors
pietroalbini
added a commit
to pietroalbini/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.
|
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
| by_ref, | ||
| mutability: Mutability::Not, | ||
| }; | ||
| // if let Some(Node::Binding(pat)) = tcx.hir().find(var_id) { |
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
| // Gather the upvars of a closure, if any. | ||
| let _upvar_decls2: Vec<_> = |
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.
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 }]
|
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
| /// The upvarID contains the HIR node ID and it also contains the full path | ||
| /// leading to the member of the struct or tuple that is used instead of the | ||
| /// entire variable. | ||
| pub upvar_list: ty::UpvarListMap, |
src/librustc_mir/build/mod.rs
Outdated
| if bm == ty::BindByValue(hir::MutMutable) { | ||
| decl.mutability = Mutability::Mut; | ||
| // Gather the upvars of a closure, if any. | ||
| let upvar_decls: Vec<_> = match hir_tables.upvar_list.get(&fn_def_id) { |
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.
There was a problem hiding this comment.
will do and re-post
| @@ -19,6 +19,15 @@ use syntax_pos::Span; | |||
|
|
|||
| /////////////////////////////////////////////////////////////////////////// | |||
| // Entry point | |||
| /// During type inference, partially inferred types are | |||
There was a problem hiding this comment.
Nit: use //, not ///, as this is not a doc comment
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 //!)?
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
|
@bors r+ |
|
📌 Commit 69e4918 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 69e4918 with merge 16912382d40270ae0d302acd6aafa247bf7f405a... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
@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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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