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

Simplify resolve_lifetimes #57408

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ljedrz
Copy link
Contributor

ljedrz commented Jan 7, 2019

Obtaining hir_id is free during lowering, so I thought it might be a good idea to save it while creating hir::Lifetime and hir::GenericParam so that resolve_lifetimes can do less work.

I'm pretty sure resolve_lifetimes can be simplified further (I'd like to squash NamedRegionMap with ResolveLifetimes), but I need to sleep on it; this seems like a step in the right direction.

As an added bonus this should be a step forward for #50928.

r? @varkor

Show resolved Hide resolved src/librustc/hir/lowering.rs Outdated
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Jan 7, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:03ff83df:start=1546877652446898210,finish=1546877744167854714,duration=91720956504
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:51:46]    Compiling parking_lot_core v0.3.0
[00:51:48]    Compiling parking_lot v0.6.4
[00:51:50]    Compiling tempfile v3.0.5
[00:51:51]    Compiling rustdoc v0.0.0 (/checkout/src/librustdoc)
[00:51:55] error[E0063]: missing field `hir_id` in initializer of `rustc::hir::Lifetime`
[00:51:55]     |
[00:51:55]     |
[00:51:55] 210 |                     args.push(hir::GenericArg::Lifetime(hir::Lifetime {
[00:51:55]     |                                                         ^^^^^^^^^^^^^ missing `hir_id`

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ljedrz ljedrz force-pushed the ljedrz:simplify_resolve_lifetimes branch from 36cadcb to 0aafea8 Jan 7, 2019

Show resolved Hide resolved src/librustc/hir/lowering.rs Outdated

@ljedrz ljedrz force-pushed the ljedrz:simplify_resolve_lifetimes branch from 0aafea8 to a82a741 Jan 8, 2019

@@ -538,6 +539,7 @@ pub enum GenericParamKind {
#[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
pub struct GenericParam {
pub id: NodeId,

This comment has been minimized.

@varkor

varkor Jan 9, 2019

Member

Can we avoid having NodeId at all? Then this would be a clear win. Have you looked at the remaining uses of id for GenericParam and Lifetime?

This comment has been minimized.

@ljedrz

ljedrz Jan 9, 2019

Contributor

I have, and there were quite a few. I can try giving it a shot 👍.

This comment has been minimized.

@ljedrz

ljedrz Jan 9, 2019

Contributor

@varkor Actually this is super complex; NodeId is still used in lots of places and just removing it causes lots of breakage, making it hard to debug. I can see that most Hir objects still point to NodeId, probably for the same reason. I have a suggestion: I will try to migrate methods working on NodeId to HirId in following PRs; at one point the compiler should just tell us that the NodeId field is not used anymore - that approach should be a lot cleaner.

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Jan 9, 2019

Do you think the extra memory required to hold hir_id would be noticeable in the perf? Maybe it's not a big deal; otherwise, of course, some other way of cracking this would be advisable.

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Jan 11, 2019

FWIW, in the meantime I'm trying to go HARD (the only way possible tbh) on HirIdification; if I succeed, most of the NodeIds in Hir items should become obsolete.

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Jan 13, 2019

Closing in favor of #57578.

@ljedrz ljedrz closed this Jan 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment