Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix broken type parameter indexing logic in wfcheck #36119

Merged
merged 1 commit into from
Sep 4, 2016

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Aug 29, 2016

r? @eddyb

Fixes #36075

}

impl From<ty::EarlyBoundRegion> for Parameter {
fn from(param: ty::EarlyBoundRegion) -> Self { Parameter(param.index) }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these entirely needed? Seems to me like it's less work to just use the index from wherever it first is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had enough index mixups.

@arielb1
Copy link
Contributor Author

arielb1 commented Aug 29, 2016

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Aug 29, 2016

📌 Commit ac345bc has been approved by eddyb

@@ -141,13 +146,14 @@ pub fn setup_constraining_predicates<'tcx>(predicates: &mut [ty::Predicate<'tcx>
// * <U as Iterator>::Item = T
// * T: Debug
// * U: Iterator
debug!("setup_constraining_predicates: predicates={:?} impl_trait_ref={:?} input_parameters={:?}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other debug! below are > 100 columns, fails tidy.

@eddyb
Copy link
Member

eddyb commented Aug 29, 2016

@bors r- tidy failed

@arielb1
Copy link
Contributor Author

arielb1 commented Aug 30, 2016

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Aug 30, 2016

📌 Commit dd72b6b has been approved by eddyb

pub enum Parameter {
Type(ty::ParamTy),
Region(ty::EarlyBoundRegion),
pub struct Parameter(pub u32);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please leave a comment explaining what this field is?

Also, I think the field should be private, and you should write fn from_index(usize) -> Parameter; it's always a shame when the public API doesn't expose usize, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ParamTy has this as a u32.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's an internal conversion anyway...but I'd argue that's a problem with ParamTy. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway whatever. I don't care that much.

@nikomatsakis
Copy link
Contributor

Left some nits. I won't stop bors since this fixes a regression, but @arielb1 if you get a chance to address them it'd be nice.

@bors
Copy link
Contributor

bors commented Sep 2, 2016

⌛ Testing commit dd72b6b with merge 6212e92...

@bors
Copy link
Contributor

bors commented Sep 2, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Sep 3, 2016

⌛ Testing commit dd72b6b with merge 70598e0...

bors added a commit that referenced this pull request Sep 3, 2016
fix broken type parameter indexing logic in wfcheck

r? @eddyb

Fixes #36075
@bors bors merged commit dd72b6b into rust-lang:master Sep 4, 2016
bors added a commit that referenced this pull request Sep 7, 2016
re-add accidentally removed line in wfcheck

Fixes #36299, introduced in #36119.

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants