Conversation
kennytm
added
the
S-waiting-on-review
|
I haven't reported this as an issue yet, but immovable generators can't be used in |
Zoxc
force-pushed
the
gen-fixes
branch
2 times, most recently
from
23addfb to
e6ec6a5
Compare
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Mostly looks good. The last commit confused me a bit though. Left some requests for comments and other nits; let me know and will re-review.
| } | ||
|
|
||
| fn visit_pat(&mut self, pat: &'tcx Pat) { | ||
| intravisit::walk_pat(self, pat); |
There was a problem hiding this comment.
What is motivating these changes? Presumably this is fixing a bug? If so, it'd be nice to have a test.
There was a problem hiding this comment.
This makes the visitation match the visitation in RegionResolutionVisitor. I don't think this matters for patterns. It might mess up generators inside patterns somehow though.
There was a problem hiding this comment.
Nit: this comment seems slightly out of date, right? Maybe "Try pre-transform fields first (e.g., upvars, current state). The remaining fields (contained in field_tys) require the final optimized MIR.
There was a problem hiding this comment.
I updated the comment.
There was a problem hiding this comment.
That said, this code is basically dead code, right? That is, we don't run the type-checker after optimization? I guess it's here just in case we do, as a kind of sanity check? (But in that case, i'm not sure that the full set of type checks would pass...)
There was a problem hiding this comment.
When I wrote it, only the InstCombine pass broke type checking. I disabled that and ran MIR type checking later anyway, for debugging purposes.
There was a problem hiding this comment.
Can we add a comment explaining what this analysis does and its overall purpose?
There was a problem hiding this comment.
I added a comment here.
There was a problem hiding this comment.
I'd appreciate a code example here, or somewhere =), explaining the meaning of these bits in a more intuitive way
There was a problem hiding this comment.
I added some more comments. Let me know if that is good enough.
There was a problem hiding this comment.
Can you add a comment explaining what this test is testing? My guess is that it's a local variable that wouldn't have to be considered live (because it is borrowed only after the yield)?
There was a problem hiding this comment.
Or maybe I'm just confused.
There was a problem hiding this comment.
I added a comment here.
|
@bors r+ |
|
📌 Commit 6c66e11 has been approved by |
nikomatsakis
added
S-waiting-on-bors
Generator bugfixes r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
5 participants
r? @nikomatsakis