Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upintroduce `-Znll-facts` to dump base-facts for the NLL analysis #50370
Conversation
rust-highfive
assigned
pnkfelix
May 1, 2018
rust-highfive
added
the
S-waiting-on-review
label
May 1, 2018
This comment has been minimized.
This comment has been minimized.
|
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
force-pushed the
nikomatsakis:nll-alias-analysis-flat
branch
from
dc7b664
to
5ef8c30
May 1, 2018
pnkfelix
reviewed
May 1, 2018
| // `killed(B,P)` when some prefix of the path borrowed at B is assigned at point P | ||
| crate killed: Vec<(BorrowIndex, LocationIndex)>, | ||
|
|
||
| // `outlives(R1, R2, P)` when we require `R1@P: R2@P` |
This comment has been minimized.
This comment has been minimized.
pnkfelix
May 1, 2018
Member
why didn't you write R1: R2 @ P here? I thought the idea was that the :-relation was indexed on P (and other occurrences of the :-relation is consistent with that interpretation ... ?)
This comment has been minimized.
This comment has been minimized.
|
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 |
pnkfelix
reviewed
May 1, 2018
| /// is the `P` value. | ||
| pub at_location: Location, | ||
| pub enum Locations { | ||
| All, |
This comment has been minimized.
This comment has been minimized.
pnkfelix
May 1, 2018
Member
so, I'm torn. I was struck by your occasional use of .unwrap_or(Location::START), because I wasn't clear on how that was justified. But then I looked at the code prior to this PR, and do see places where you have (quite naturally) replaced previous uses of START with All...
Anyway, reading over those cases led me back to this enum declaration. I thought at first that All is pretty self-documenting here in the enum declaration itself (i.e., it represents all locations, and it is used in cases where you are describing some constraint that is true everywhere in the control-flow graph, like type equivalences)... But then after reading the comments that go with Locations::Pair, I sort of lost that perspective. I guess the main point is that some constraints are flow-independent throughout the function, and others are control-flow dependent, and that is the distinction encoded in this enum? Does that sound right?
But then ... how should one decide when it is appropriate to map from_location() (or at_location()) of None to Location::START? Is it just that since we're talking about an flow-independent constraint, then any location will do, so we might as well pick START?
This comment has been minimized.
This comment has been minimized.
nikomatsakis
May 2, 2018
Author
Contributor
TBH, the code that "falls back" to START is a bit confusing. I'm not 100% convinced that is a correct thing to do, but I was just trying to keep existing behavior. In any case, START is a better choice for the old NLL because all other points are reachable from there. I can try to elaborate the comment a bit.
The problem I was specifically encountering here in the new analysis was that we would do this:
_0 = p
where _0 is the return pointer. We treat _0 like any other place, and hence we considered _0 to not be live until that point! This meant that when we related _0 to the return type at the entry, all of those subtype relations were lost. This is why I re-add it at each point. TBH it's .. maybe a hack? I wasn't fully satisfied.
This comment has been minimized.
This comment has been minimized.
|
I had some small nits mostly related to comments. But it looks fine as far as I understand it. r=me once it passes travis. |
nikomatsakis
force-pushed the
nikomatsakis:nll-alias-analysis-flat
branch
from
a8160b0
to
2a240b1
May 4, 2018
This comment has been minimized.
This comment has been minimized.
|
Rebased and added a comment concerning x.py test passes locally. @bors r=pnkfelix |
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-review
labels
May 4, 2018
nikomatsakis
force-pushed the
nikomatsakis:nll-alias-analysis-flat
branch
from
ecb2af4
to
047ad85
May 4, 2018
This comment has been minimized.
This comment has been minimized.
|
@bors r=pnkfelix |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
@bors r- ↑ |
bors
added
S-waiting-on-author
and removed
S-waiting-on-bors
labels
May 4, 2018
nikomatsakis
added some commits
May 1, 2018
nikomatsakis
force-pushed the
nikomatsakis:nll-alias-analysis-flat
branch
from
047ad85
to
b36cbcb
May 5, 2018
This comment has been minimized.
This comment has been minimized.
|
Rebased, fixed resulting breakage. @bors r=pnkfelix |
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-author
labels
May 5, 2018
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
May 5, 2018
This comment has been minimized.
This comment has been minimized.
|
|
nikomatsakis commentedMay 1, 2018
r? @pnkfelix