Rollup of 6 pull requests #40752
Merged
Rollup of 6 pull requests #40752
Conversation
This file contains 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
Member
frewsxcv
commented
Mar 23, 2017
frewsxcv
commented
- Successful merges: Teach rustc --emit=mir #39891, Introduce HirId, a replacement for ast::NodeId after lowering to HIR #40518, Correctly get source for metatdata-only crate type #40542, Update gcc used for dist-x86-linux builds #40617, Some preparations for directly computing the ICH of crate-metadata. #40678, Remove unused adt-def insertion by constructor DefIndex #40696
- Failed merges:
replace `unwrap()` with `expect()`
HirId has a more stable representation than NodeId, meaning that modifications to one item don't influence (part of) the IDs within other items. The other part is a DefIndex for which there already is a way of stable hashing and persistence. This commit introduces the HirId type and generates a HirId for every NodeId during HIR lowering, but the resulting values are not yet used anywhere, except in consistency checks.
This way we can have all item-likes occupy a dense range of DefIndexes, which is good for making fast, array-based dictionaries.
Teach rustc --emit=mir I'm opening this PR to discuss: 1. Is this a good idea? 1. Is this a good implementation? I'm sure people will have opinions on both points! This spawned from rust-lang#31847 (comment), so I figured a prototype implementation could help provide a seed to talk about.
Introduce HirId, a replacement for ast::NodeId after lowering to HIR This is the first step towards implementing rust-lang#40303. This PR introduces the `HirId` type and generates a `HirId` for everything that would be assigned one (i.e. stuff in the HIR), but the HIR data types still use `NodeId` for now. Changing that is a big refactoring that I want to do in a separate PR. A `HirId` uniquely identifies a node in the HIR of the current crate. It is composed of the `owner`, which is the `DefIndex` of the directly enclosing `hir::Item`, `hir::TraitItem`, or `hir::ImplItem` (i.e. the closest "item-like"), and the `local_id` which is unique within the given owner. This PR is also running a number of consistency checks for the generated `HirId`s: - Does `NodeId` in the HIR have a corresponding `HirId`? - Is the `owner` part of each `HirId` consistent with its position in the HIR? - Do the numerical values of the `local_id` part all lie within a dense range of integers? cc @rust-lang/compiler r? @eddyb or @nikomatsakis
Correctly get source for metatdata-only crate type Closes rust-lang#40535 However, I'm not sure how to approach writing a regression test since I'm still working on a reduced test case from the code that caused the ICE in the first place. It's not enough to have an unknown `extern crate` in a metadata crate, it depends on a few extra arguments but I'm not sure which yet. Also replaced the `unwrap()` with a more informative `expect()`. r? @jseyfried
Update gcc used for dist-x86-linux builds GCC 4.7 is too old to build LLVM 4.0, so this PR updates to 4.8. r? @alexcrichton (I'll ping you again once travis is green and the test commit is removed).
…tsakis Some preparations for directly computing the ICH of crate-metadata. This PR contains some small fixes in preparation for direct metadata hashing. It mostly just moves stuff into places where it will be needed (making the module structure slightly cleaner along the way) and it fixes some omissions in the MIR region eraser. r? @nikomatsakis
…, 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
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
@bors r+ p=10 |
|
📌 Commit 9307418 has been approved by |
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Mar 23, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.