-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Closed
Labels
A-NLLArea: Non-lexical lifetimes (NLL)Area: Non-lexical lifetimes (NLL)NLL-diagnosticsWorking towards the "diagnostic parity" goalWorking towards the "diagnostic parity" goal
Milestone
Description
Consider:
compare it to the AST borrowck output:
- Why are we inverting the order in which we report these diagnostics?
- We may want to consider not doing that. Even if it means e.g. buffering the diangotics and sorting them before emitting them, or some similar hack. (I admit I am loathe to do this since I myself really like when I am able to correlate the
RUST_LOG
output with diagnostic output. But if necessary, we could add a-Z
flag to turn off the buffer+sorting, in order to bring back such correlation. For end users, buffering+sorting is probably a net win.)
An important reason to prioritize this: Fixing this is likely going to make our internal process for comparing the diagnostic output more efficient.
Metadata
Metadata
Assignees
Labels
A-NLLArea: Non-lexical lifetimes (NLL)Area: Non-lexical lifetimes (NLL)NLL-diagnosticsWorking towards the "diagnostic parity" goalWorking towards the "diagnostic parity" goal
Type
Projects
Relationships
Development
Select code repository
Activity
nikomatsakis commentedon Jun 29, 2018
Marking as Edition Preview 2 because .. maybe an easy fix.
nikomatsakis commentedon Jul 3, 2018
I think we should just try visiting the basic blocks in reverse-postfix order. The visit occurs here:
rust/src/librustc_mir/dataflow/mod.rs
Lines 334 to 340 in 860d169
and in particular the for loop here goes over the basic blocks in an arbitrary order
rust/src/librustc_mir/dataflow/mod.rs
Line 336 in 860d169
What we want to do is to go over in reverse post-order, which basically maps to execution order as we would normally define it. There is a function
reverse_postorder
that already exists to compute this ordering -- it returns anIterator<Item = BasicBlock>
-- and you can see some other random code that uses it here:rust/src/librustc_mir/transform/qualify_consts.rs
Line 130 in 860d169
So we basically want to change that code to iterate over the result of
reverse_postorder
.nikomatsakis commentedon Jul 6, 2018
OK, @csmoe did the RPO trick -- it definitely helped, but it doesn't seem to have fixed @pnkfelix's example. Not sure @pnkfelix how many more such examples there are -- however, it occurs to me -- @spastorino is also adding some buffering that we might use to do sorting after the fact, as well.
Rollup merge of rust-lang#52067 - csmoe:issue-51167, r=nikomatsakis
nikomatsakis commentedon Jul 17, 2018
Assigning to @spastorino as this should get fixed as a side-effect of #46908
spastorino commentedon Jul 23, 2018
Assigning to @pnkfelix as he took over the migrate thing. I can help here if needed at some point.
pnkfelix commentedon Jul 24, 2018
@nikomatsakis do you think this should remain on EP2, or should we move it to RC?
(I'm going to take a shot at resolving it today, or at least doing as much as I can via the buffering we added. But still, time is tight for EP2.)
pnkfelix commentedon Jul 24, 2018
moving this to RC as I think it is sufficiently low priority that it should not block EP2 nor NLL's release in EP2.
Rollup merge of rust-lang#52904 - pnkfelix:issue-51167-sort-by-span, …
Rollup merge of rust-lang#52904 - pnkfelix:issue-51167-sort-by-span, …