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

Mb/36812 ich function interfaces #36974

Merged

Conversation

MathieuBordere
Copy link

r? @michaelwoerister

This PR contains fixes for #36812 and #36914

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@michaelwoerister
Copy link
Member

Thank you so much! I'll take a look at this ASAP.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

This is excellent work! I like how you included SawTyComponent even though we didn't explicitly talk about it. I think this has already caught quite a few bugs before we had to find out about them the hard way.

I've left a few comments about things that need to change. But this almost ready to merge.

// Change Parameter Order ------------------------------------------------------

#[cfg(cfail1)]
fn order_of_parameters(p1: i32, p2: i32) {}
Copy link
Member

Choose a reason for hiding this comment

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

Could you give p2 a different type than p1?

// Return Impl Trait -----------------------------------------------------------

#[cfg(cfail1)]
fn return_impl_trait() -> i32 {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it would make more sense to have it change from impl Clone to impl Copy instead? It's probably best to add a separate test case for that.

// Second Lifetime Bound -------------------------------------------------------

#[cfg(cfail1)]
fn second_lifetime_bound<'a, T: 'a>() {}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add 'b also to the cfail1 version so that we are really just testing for the addition of a bound (as opposed to a bound and a new lifetime parameter)?

// Inline Never ----------------------------------------------------------------

#[cfg(cfail1)]
fn inline_never() {}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an #[inline(always)] attribute to the cfail1 version? I'd like this to test whether changing the kind of inline hint is detected.

#[rustc_clean(label="Hir", cfg="cfail3")]
#[rustc_metadata_dirty(cfg="cfail2")]
#[rustc_metadata_clean(cfg="cfail3")]
#[linkage]
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this compiles :)
Can you change that to #[linkage="weak_odr"] so we actually specify an explicit linkage?

// Lifetime Bound --------------------------------------------------------------

#[cfg(cfail1)]
fn lifetime_bound<T>() {}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add 'a to the cfail1 version too? (See a similar case with the second lifetime bound)

SawTyArray,
SawTyPtr(Mutability),
SawTyRptr(Mutability),
SawTyBareFn,
Copy link
Member

Choose a reason for hiding this comment

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

The BareFnTy contained in TyBareFn also contains an unsafety and an abi field that should be added here.

@MathieuBordere
Copy link
Author

Thanks, will incorporate your remarks.

@@ -366,7 +366,10 @@ fn saw_ty(node: &Ty_) -> SawTyComponent {
TyArray(..) => SawTyArray,
TyPtr(ref mty) => SawTyPtr(mty.mutbl),
TyRptr(_, ref mty) => SawTyRptr(mty.mutbl),
TyBareFn(..) => SawTyBareFn,
TyBareFn(ref barefnty) => {
let ref fnty = *barefnty;
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant. Can't you just write SawTyBareFn(barefnty.unsafety, barefnty.abi) in the line below? Should do the same thing.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I'm very new to Rust, still a lot to learn :)

@michaelwoerister
Copy link
Member

Thanks, I think all my comments have been addressed.

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2016

📌 Commit 0e40dbb has been approved by michaelwoerister

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Oct 6, 2016
…Interfaces, r=michaelwoerister

Mb/36812 ich function interfaces

r? @michaelwoerister

This PR contains fixes for rust-lang#36812 and rust-lang#36914
bors added a commit that referenced this pull request Oct 6, 2016
@bors bors merged commit 0e40dbb into rust-lang:master Oct 6, 2016
@michaelwoerister
Copy link
Member

🎉

@MathieuBordere
Copy link
Author

Feels nice to be able to contribute, up to the next! Thanks!

@MathieuBordere MathieuBordere deleted the mb/36812_ICHFunctionInterfaces branch October 7, 2016 19:22
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 11, 2016
…ests, r=nikomatsakis

ICH: Enable some cases in trait definition hashing.

Enable some test cases originally written by @eulerdisk. The tests can be enabled now because @MathieuBordere has fixed the underlying problem in rust-lang#36974.

r? @nikomatsakis
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.

4 participants