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

ICE Fix #16562 #16912

Closed
wants to merge 1 commit into from
Closed

ICE Fix #16562 #16912

wants to merge 1 commit into from

Conversation

wickerwaka
Copy link

Fixes #16562

Detect ty_err in fixup_substs and return None instead of failing.
In search_for_vtable return vtable_error in both early and non-early
if fixup_substs returns None. This was asserting previously. I don't
know anything about the early vs. non-early stages and whether this
is significant or not.

Reduced the amount of data being printed to the debug log in
tydecode.rs. It was printing several megabytes of metadata.

Detect ty_err in fixup_substs and return None instead of failing.
In search_for_vtable return vtable_error in both early and non-early
if fixup_substs returns None. This was asserting previously. I don't
know anything about the early vs. non-early stages and whether this
is significant or not.

Reduced the amount of data being printed to the debug log in
tydecode.rs. It was printing several megabytes of metadata.

Added license text to test
@wickerwaka
Copy link
Author

Added the license header, thanks.

@brson
Copy link
Contributor

brson commented Sep 9, 2014

@pnkfelix Does this look good to you?

@brson
Copy link
Contributor

brson commented Sep 9, 2014

@wickerwaka Nice fix! Sorry for the delay.

@alexcrichton
Copy link
Member

ping @pnkfelix

_ => fail!("t_f should be a trait")
}
})
match fixup_ty(vcx, span, t, is_early) {
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 like you manually inlined the effect of map ... am I correct? Was that necessary?

@pnkfelix
Copy link
Member

Ah, the codebase has changed significantly since this was written. I think this ICE is gone now; the test you have here now does not iCE.

I checked with nmatsakis; he says the line-limit in metadata::tydecode is reasonable.

It seems like a rebase is going to amount to:

  1. putting in this debug log line limit in metadata::tydecode
  2. adding a test case for the original ICE.

If you are willing to rebase, (and I hope you are), then that can be considered r+'ed by me. Bonus points if you actually split the rebased PR into two commits (since the two items in my list above are 100% orthogonal to each other, in my opinion).

@pnkfelix
Copy link
Member

@wickerwaka (sorry for the delay in the review. obviously things changed in the meantime.)

If you want to do the rebase suggested above, great. If you don't feel like bothering, please just drop me a line and I'll try to salvage what I want from this.

@Gankra
Copy link
Contributor

Gankra commented Nov 8, 2014

@wickerwaka Any news on this? @pnkfelix You might just have to take point here.

@wickerwaka
Copy link
Author

@gankro, Sorry, real life getting in the way. I'll take a look at this.

@wickerwaka
Copy link
Author

Closing this, everything other than the metadata debug output has been addressed by other changes. Opened a new PR with the metadata stuff: PR #19043

@wickerwaka wickerwaka closed this Nov 17, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2024
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.

ICE: With (unnecessary) extra generic argument
5 participants