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

Remove ErasedRegions from substs #32346

Merged
merged 3 commits into from
Mar 25, 2016
Merged

Conversation

nikomatsakis
Copy link
Contributor

This commit removes the ErasedRegions enum from Substs. Instead, in trans, we just generate a vector of ReStatic of suitable length. The goal is both general cleanup and to help pave the way for a glorious future where erasure is used in type check.

r? @eddyb

One concern: might be nice to do some profiling. Not sure the best way to do that. Perhaps I'll investigate running nrc's test suite locally.


/// Given the node-id of some local item that has no type
/// parameters, make a suitable "empty substs" for it.
pub fn empty_substs_for_node_id(&self, item_node_id: ast::NodeId)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want this function, all callers already compute local_def_id themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, so they do.

On Fri, Mar 18, 2016 at 8:59 PM, Eduard-Mihai Burtescu <
[email protected]> wrote:

In src/librustc_trans/trans/context.rs
#32346 (comment):

@@ -856,6 +855,29 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
codegen_items.insert(cgi, TransItemState::NotPredictedButGenerated);
}
}
+

  • /// Given the node-id of some local item that has no type
  • /// parameters, make a suitable "empty substs" for it.
  • pub fn empty_substs_for_node_id(&self, item_node_id: ast::NodeId)

I don't think we want this function, all callers already compute
local_def_id themselves.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/32346/files/378743fd079475246589c1d4ddb7b614ab58f9cc..d8c2e42975cc77263235ea29e5ed10bbaa9bcebc#r56740326

@eddyb
Copy link
Member

eddyb commented Mar 19, 2016

r=me with that one nit fixed.

@nikomatsakis
Copy link
Contributor Author

Doing a crater run -- I feel like I could have easily missed a corner case.

@nikomatsakis
Copy link
Contributor Author

Crater run: https://gist.github.com/nikomatsakis/21a52e57ab8ec18856be

Two failures, both look like inconclusive results. I will try them locally.

@nikomatsakis
Copy link
Contributor Author

They built locally.

@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Mar 19, 2016

📌 Commit 9b03591 has been approved by eddyb

@nikomatsakis
Copy link
Contributor Author

@bors r-

actually, I wanted to measure rustc perf first

@bors
Copy link
Contributor

bors commented Mar 23, 2016

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

It no longer reads from `ast_ty_to_ty_cache`, which was very wrong. It
also correctly handles higher-ranked regions.
This hack has long since outlived its usefulness; the transition to
trans passing around full substitutions is basically done. Instead of
`ErasedRegions`, just supply substitutions with a suitable number of
`'static` entries, and invoke `erase_regions` when needed (the latter of
which we already do).
@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Mar 24, 2016

📌 Commit c5d74be has been approved by eddyb

@bors
Copy link
Contributor

bors commented Mar 24, 2016

⌛ Testing commit c5d74be with merge d7a7168...

bors added a commit that referenced this pull request Mar 24, 2016
Remove `ErasedRegions` from substs

This commit removes the `ErasedRegions` enum from `Substs`. Instead, in trans, we just generate a vector of `ReStatic` of suitable length. The goal is both general cleanup and to help pave the way for a glorious future where erasure is used in type check.

r? @eddyb

One concern: might be nice to do some profiling. Not sure the best way to do that. Perhaps I'll investigate running nrc's test suite locally.
@bors bors merged commit c5d74be into rust-lang:master Mar 25, 2016
@nikomatsakis nikomatsakis deleted the no-erased-regions branch March 30, 2016 16:16
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.

3 participants