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

ICH: Fix and test foreign mod hashing. #38479

Merged
merged 3 commits into from
Dec 28, 2016

Conversation

michaelwoerister
Copy link
Member

match i.node {
ForeignItemFn(ref fn_decl, _) => {
SawForeignItem(SawForeignItemComponent::Fn {
variadic: fn_decl.variadic
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be part of FnDecl?

Copy link
Member Author

Choose a reason for hiding this comment

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

visit_fn() is not called for foreign items, and it's not clear that it should be (since foreign items have no body like the other cases). As far as I could tell, this is the only way to get at a foreign item's FnDecl without changing the visitor implementation.

Copy link
Member

@eddyb eddyb Dec 20, 2016

Choose a reason for hiding this comment

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

Oh there is no visit_fn_decl? Maybe you should add that instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid, you're right :)

@michaelwoerister
Copy link
Member Author

I've added a visit_fn_decl method to intravisit::Visitor and looked through all occurrences of visit_fn. None of them seemed like it would need to change or could be implemented much more nicely via visit_fn_decl. So the only use of the new method is in the ICH hasher.

@michaelwoerister
Copy link
Member Author

I removed hashes for individual foreign items, which caused no trouble for the incremental tests locally. Let's see what Travis says.

@nikomatsakis
Copy link
Contributor

@michaelwoerister looks good. 👍

@nikomatsakis
Copy link
Contributor

@michaelwoerister though I feel like this caused some sort of ICE -- do you think you could use this branch to build syntex with RUSTFLAGS="-Z incremental" or something like that?

@michaelwoerister
Copy link
Member Author

do you think you could use this branch to build syntex with RUSTFLAGS="-Z incremental" or something like that?

Sure.

@michaelwoerister
Copy link
Member Author

So, I did a full bootstrap on this branch, with both stage1 and stage2 being built with -Zincremental and haven't run into any ICEs.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 27, 2016

📌 Commit f0a630b has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 28, 2016

⌛ Testing commit f0a630b with merge a9ab778...

bors added a commit that referenced this pull request Dec 28, 2016
@bors
Copy link
Contributor

bors commented Dec 28, 2016

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a9ab778 to master...

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