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

Ignore builtin template when parsing #594

Closed
wants to merge 1 commit into from

Conversation

upsuper
Copy link
Contributor

@upsuper upsuper commented Mar 20, 2017

This should fix #584.

@upsuper
Copy link
Contributor Author

upsuper commented Mar 20, 2017

This is not the only crash, apparently.

@upsuper
Copy link
Contributor Author

upsuper commented Mar 20, 2017

Ah, I guess this isn't the right fix... things still bust after this. I guess various places depend on that templates always have definition, and builtin template here breaks everything...

@emilio
Copy link
Contributor

emilio commented Mar 20, 2017

Yes, I pinged @fitzgen about this yesterday. #544 makes it worse than that unfortunately. I suspect this fix would work for the bindgen version we use in stylo right now though. Can you try backporting this to a revision before #544?

let inst = TemplateInstantiation::from_ty(&ty, ctx);
let inst = match TemplateInstantiation::from_ty(&ty, ctx) {
Some(inst) => inst,
None => return Err(ParseError::Continue),
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more appropriate to return an opaque type here, rather than continue on to who-knows-what that we know isn't quite right.

@fitzgen
Copy link
Member

fitzgen commented Mar 20, 2017

I'm building clang 4.0 and I'll poke at the test case here.

@fitzgen
Copy link
Member

fitzgen commented Mar 20, 2017

If there's a reduced test case for the other issues in the template parameter usage analysis, that would be very helpful to have as well.

@emilio
Copy link
Contributor

emilio commented Mar 20, 2017

This fixes the test case above. This fails because the cursor we're poking at is a NoDeclFound IIRC.

@fitzgen
Copy link
Member

fitzgen commented Mar 20, 2017

This fails because the cursor we're poking at is a NoDeclFound IIRC.

What is "this" here?

@emilio
Copy link
Contributor

emilio commented Mar 20, 2017

What is "this" here?

The failure that I was hitting when I looked at the same issue, was that we were not getting the specialized template because we couldn't find it from the type declaration, since it's a NoDeclFound cursor, since it's builtin. Sorry for not being more clear above :)

@emilio
Copy link
Contributor

emilio commented Apr 4, 2017

@fitzgen do you think we still want this? I think we do, unless you're working on other failures that happen to fix this too. If you are, please land the test-case along with your work!

@fitzgen
Copy link
Member

fitzgen commented Apr 4, 2017

r=me with the previous comment addressed

@fitzgen
Copy link
Member

fitzgen commented Apr 4, 2017

previous comment addressed

about returning an opaque type rather than a parse error

@fitzgen
Copy link
Member

fitzgen commented Apr 4, 2017

Rolled this + my review comment into #613

@fitzgen fitzgen closed this Apr 4, 2017
bors-servo pushed a commit that referenced this pull request Apr 4, 2017
Handle when we can't instantiate templates because we can't find a template definition

#594 + my review commetn about using opaque types
bors-servo pushed a commit that referenced this pull request Apr 4, 2017
Handle when we can't instantiate templates because we can't find a template definition

#594 + my review commetn about using opaque types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bindgen panics in Stylo on master
4 participants