small fix to closure debuginfo #38483

Merged
merged 1 commit into from Dec 21, 2016

Conversation

Projects
None yet
8 participants
@camlorn
Contributor

camlorn commented Dec 20, 2016

This is fallout from #37429. The faking we do to make closures appear to have local variables was broken and made it through CI because the debuginfo tests got turned off.

This probably needs a dedicated test, but I wanted to get this in as quickly as possible because we probably need to backport this to the beta.

r? @alexcrichton

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 20, 2016

Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Dec 20, 2016

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@camlorn camlorn changed the title from This fixes closure debuginfo so that the local variable faking we do works again. Fallout from #37429. to small fix to closure debuginfo Dec 20, 2016

@camlorn

This comment has been minimized.

Show comment
Hide comment
@camlorn

camlorn Dec 20, 2016

Contributor

I just hit the wrong button and submitted this without a description, sorry.

EDIT(@eddyb): moved the rest of this comment to the PR description.

Contributor

camlorn commented Dec 20, 2016

I just hit the wrong button and submitted this without a description, sorry.

EDIT(@eddyb): moved the rest of this comment to the PR description.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 20, 2016

Member

r? @eddyb

seems ok to me, but I'm probably not best to review

Member

alexcrichton commented Dec 20, 2016

r? @eddyb

seems ok to me, but I'm probably not best to review

@eddyb

This comment has been minimized.

Show comment
Hide comment
Member

eddyb commented Dec 20, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2016

Contributor

📌 Commit e1d8806 has been approved by eddyb

Contributor

bors commented Dec 20, 2016

📌 Commit e1d8806 has been approved by eddyb

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 20, 2016

Member

@bors: p=1

fixes a regression, so I think we'll want to land this soon hopefully

Member

alexcrichton commented Dec 20, 2016

@bors: p=1

fixes a regression, so I think we'll want to land this soon hopefully

bors added a commit that referenced this pull request Dec 20, 2016

@bors bors merged commit e1d8806 into rust-lang:master Dec 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 30, 2016

Contributor

Marking as beta-accepted. Small (if...dense) patch, regression. cc @rust-lang/compiler

Contributor

nikomatsakis commented Dec 30, 2016

Marking as beta-accepted. Small (if...dense) patch, regression. cc @rust-lang/compiler

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 30, 2016

Contributor

Definitely we want a test here...

Contributor

nikomatsakis commented Dec 30, 2016

Definitely we want a test here...

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Dec 30, 2016

Member

@nikomatsakis IIRC we had a test but it wasn't running on the bots, @alexcrichton knows more.

Member

eddyb commented Dec 30, 2016

@nikomatsakis IIRC we had a test but it wasn't running on the bots, @alexcrichton knows more.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 30, 2016

Member

Oh yes #37429 should have been prevented from landing in the first place due to it causing a regression to an existing test that this PR fixes. That test was erroneously not running and has since been fixed to run on all PRs.

In that sense I believe we've already got tests checked in for this.

Member

alexcrichton commented Dec 30, 2016

Oh yes #37429 should have been prevented from landing in the first place due to it causing a regression to an existing test that this PR fixes. That test was erroneously not running and has since been fixed to run on all PRs.

In that sense I believe we've already got tests checked in for this.

@camlorn

This comment has been minimized.

Show comment
Hide comment
@camlorn

camlorn Dec 30, 2016

Contributor

I'm not sure what dense means but suspect it is bad; if you're referring to the description and the commit it's because I don't fully understand what's going on. @eddyb gets credit for this fix, and they tried to explain but not with much luck, I'm afraid.

I forget which test specifically checks this, but there are already tests examining closure-local variables.

Contributor

camlorn commented Dec 30, 2016

I'm not sure what dense means but suspect it is bad; if you're referring to the description and the commit it's because I don't fully understand what's going on. @eddyb gets credit for this fix, and they tried to explain but not with much luck, I'm afraid.

I forget which test specifically checks this, but there are already tests examining closure-local variables.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 30, 2016

Contributor

@camlorn I didn't mean dense in a bad way, just meant that there are few lines changed, but the correctness of the changes is non-obvious -- although re-reading they actually look pretty simple.

Contributor

nikomatsakis commented Dec 30, 2016

@camlorn I didn't mean dense in a bad way, just meant that there are few lines changed, but the correctness of the changes is non-obvious -- although re-reading they actually look pretty simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment