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

Pass the potential id to be used as the type wrapper id, and prevent leaving a dangling a type parsing template aliases. #105

Merged
merged 6 commits into from
Oct 29, 2016

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Oct 21, 2016

This should fix the MSVC problems @upsuper has been having.

@upsuper, can you confirm if this fixes the panic you were hitting?

@emilio
Copy link
Contributor Author

emilio commented Oct 21, 2016

r? @nox

@upsuper
Copy link
Contributor

upsuper commented Oct 22, 2016

No, it doesn't seem to fix that issue.

@emilio
Copy link
Contributor Author

emilio commented Oct 22, 2016

Maybe the next one does, I'm trying to build a test-case now.

@emilio emilio force-pushed the msvc branch 2 times, most recently from 75388f5 to 587b54c Compare October 22, 2016 01:54
@emilio emilio changed the title Pass the potential id to be used as the type wrapper id. Pass the potential id to be used as the type wrapper id, and prevent leaving a dangling a type parsing template aliases. Oct 22, 2016
@bors-servo
Copy link

☔ The latest upstream changes (presumably #110) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio
Copy link
Contributor Author

emilio commented Oct 23, 2016

Rebased (a while ago). r? @nox

@emilio
Copy link
Contributor Author

emilio commented Oct 24, 2016

(btw, the second patch makes the MSVC panic go away)

@emilio
Copy link
Contributor Author

emilio commented Oct 27, 2016

Actually, @fitzgen, may you also be interested in reviewing this? :)

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

In general, things make sense, as much as they can.

You mention in the title that this prevents a dangling type, but I don't see any new test case (hint hint).

This makes me think we should have a Trace trait for our IR types that gives all the ItemId a given IR thing has references to, and then we can walk the full IR and ensure that we don't have dangling ItemId references. Debug only, ofc.

@emilio
Copy link
Contributor Author

emilio commented Oct 29, 2016

Yep, added a test.

The Trace thing sounds good, it mostly duplicates TypeCollector, so it'd be nice to share logic there.

I also added a test for #3, which is r=me.

@bors-servo: r=fitzgen,emilio

@bors-servo
Copy link

📌 Commit e04eda1 has been approved by fitzgen,emilio

@highfive highfive assigned fitzgen and unassigned nox Oct 29, 2016
@bors-servo
Copy link

⌛ Testing commit e04eda1 with merge 6ff1c1d...

bors-servo pushed a commit that referenced this pull request Oct 29, 2016
Pass the potential id to be used as the type wrapper id, and prevent leaving a dangling a type parsing template aliases.

This should fix the MSVC problems @upsuper has been having.

@upsuper, can you confirm if this fixes the panic you were hitting?
@bors-servo
Copy link

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit e04eda1 into rust-lang:master Oct 29, 2016
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.

6 participants