Remove unused adt-def insertion by constructor DefIndex#40696
Merged
bors merged 2 commits intorust-lang:masterfrom Mar 23, 2017
Merged
Remove unused adt-def insertion by constructor DefIndex#40696bors merged 2 commits intorust-lang:masterfrom
bors merged 2 commits intorust-lang:masterfrom
Conversation
Member
Author
|
cc #40614 |
Contributor
|
I can confirm, this is not necessary now. This "constructor insertion" was done in two places - librustc_metadata/decoder.rs and librustc_typeck/collect.rs, could you make sure both are removed? |
Member
Author
|
@petrochenkov Was that the one you were thinking of? |
Contributor
Collaborator
|
📌 Commit 239da92 has been approved by |
Collaborator
|
🔒 Merge conflict |
bstrie
force-pushed
the
remove-unused-adt-def-code
branch
from
239da92 to
873248d
Compare
Contributor
|
I've rebased this branch due to a submodule conflict that invalidated many PRs. @bors r=petrochenkov |
Collaborator
|
📌 Commit 873248d has been approved by |
6 tasks
frewsxcv
added a commit
to frewsxcv/rust
that referenced
this pull request
Mar 23, 2017
…, r=petrochenkov Remove unused adt-def insertion by constructor DefIndex It looks to me like ADT definitions weren't being looked up by constructor id, and a test run supports my theory. In any case, I'm not sure it would have worked in its current configuration. If I understand correctly, the `adt_def` map entry from constructor id -> adt def would only be present after a successful call to `queries::adt_def::get` with the proper ADT `DefIndex`. Trying to look up an adt_def by the constructor index prior to a successful lookup by ADT index would fail since `item.kind` would be `EntryKind::Fn` (for the constructor function) and so would trigger the `bug!`. r? @nikomatsakis
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It looks to me like ADT definitions weren't being looked up by constructor id, and a test run supports my theory.
In any case, I'm not sure it would have worked in its current configuration. If I understand correctly, the
adt_defmap entry from constructor id -> adt def would only be present after a successful call toqueries::adt_def::getwith the proper ADTDefIndex. Trying to look up an adt_def by the constructor index prior to a successful lookup by ADT index would fail sinceitem.kindwould beEntryKind::Fn(for the constructor function) and so would trigger thebug!.r? @nikomatsakis