rustc_resolve: Refactor away NameBindings and ImportResolutionPerNamespace by jseyfried · Pull Request #30843 · rust-lang/rust

jseyfried · 2016-01-12T12:21:32Z

This commit refactors the field Module::children from mapping Name -> NameBindings to mapping (Name, Namespace) -> NameBinding and refactors the field Module::import_resolutions from mapping Name -> ImportResolutionPerNamespace to mapping (Name, Namespace) -> ImportResolution.

This allows the duplicate checking code to be refactored so that NameBinding no longer needs ref-counting or a RefCell (removing the need for NsDef).

r? @nikomatsakis

jseyfried · 2016-01-13T00:38:26Z

I amended the commit to add a couple of comments and refactored some more code out of resolve_single_import.

bors · 2016-01-14T13:46:29Z

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

bors · 2016-01-15T14:40:04Z

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

nikomatsakis · 2016-01-15T19:59:06Z

Sorry for the delay in reviewing this! Working on it. Been a crazy week.

bors · 2016-01-21T03:35:07Z

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

bors · 2016-01-22T00:33:13Z

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

jseyfried · 2016-01-24T10:06:10Z

@nikomatsakis No hurry, those first two upstream changes were PRs I backported from this branch and the other two didn't interact with this PR that much.

Let me know if you'd like me to explain, motivate, or justify the purity of anything in this refactoring.

I also have a branch a commit ahead of this one (diff) where I refactor away Module's external_module_children and another branch a commit after that (diff) where I fix issue #26775 by a "proper implementation" of what @nrc describes here, except with a small difference that significantly reduces breakage.

nrc · 2016-01-25T16:08:44Z

cc @petrochenkov

nikomatsakis · 2016-01-25T17:06:21Z

@jseyfried as you can tell I still haven't gotten to it :) I hope to do so soon though. Last week was just a doozy. (The truth is, I don't know resolve as well as I would like, so your PRs always require me to read into the code quite a bit.)

UPDATE: But it's good for me. :)

petrochenkov · 2016-01-26T20:01:06Z

build_reduced_graph.rs looks good to me

jseyfried · 2016-01-27T02:52:45Z

@petrochenkov good points, thanks. I implemented your suggestions in the third commit.

petrochenkov · 2016-01-27T19:17:13Z

jseyfried@521aaf2 reviewed, all the code seems to be moved correctly, it's hard to track though

nikomatsakis · 2016-01-28T17:39:01Z

OK, I feel pretty good about this change. @petrochenkov, your thoughts?

petrochenkov · 2016-01-28T18:37:45Z

your thoughts?

resolve becomes 450 lines less scary, merge of course!

bors · 2016-01-29T15:40:47Z

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

nikomatsakis · 2016-01-29T21:08:20Z

OK, r=me after a rebase. Just for fun, I kicked off a crater run as well (resolve tends to be finicky).

…icateCheckingMode, and NamespaceDefinition.

jseyfried · 2016-01-29T23:29:08Z

rebased

nikomatsakis · 2016-01-30T01:13:57Z

Crater report shows zero regressions: https://gist.github.com/nikomatsakis/5181cff5cc53438ab82e

nikomatsakis · 2016-01-30T01:14:02Z

@bors r+

bors · 2016-01-30T01:14:02Z

📌 Commit afd42d2 has been approved by nikomatsakis

bors · 2016-01-30T03:17:25Z

⌛ Testing commit afd42d2 with merge 9ba9f0a...

bors · 2016-01-30T03:42:50Z

💔 Test failed - auto-mac-64-nopt-t

alexcrichton · 2016-01-30T03:53:52Z

@bors: retry

On Fri, Jan 29, 2016 at 7:42 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/7905
—
Reply to this email directly or view it on GitHub
#30843 (comment).

bors · 2016-01-30T04:13:28Z

⌛ Testing commit afd42d2 with merge 558fd51...

bors · 2016-01-30T04:48:28Z

💔 Test failed - auto-linux-cross-opt

alexcrichton · 2016-01-30T07:12:06Z

@bors: retry

On Fri, Jan 29, 2016 at 8:48 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-cross-opt
http://buildbot.rust-lang.org/builders/auto-linux-cross-opt/builds/1063
—
Reply to this email directly or view it on GitHub
#30843 (comment).

bors · 2016-01-30T10:16:56Z

⌛ Testing commit afd42d2 with merge b16fbe7...

@nikomatsakis

This commit refactors the field `Module::children` from mapping `Name` -> `NameBindings` to mapping `(Name, Namespace)` -> `NameBinding` and refactors the field `Module::import_resolutions` from mapping `Name` -> `ImportResolutionPerNamespace` to mapping `(Name, Namespace)` -> `ImportResolution`. This allows the duplicate checking code to be refactored so that `NameBinding` no longer needs ref-counting or a RefCell (removing the need for `NsDef`). r? @nikomatsakis

bors · 2016-01-30T12:32:45Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned nikomatsakis Jan 12, 2016

jseyfried force-pushed the no_per_ns branch 5 times, most recently from a778203 to 07df703Compare January 13, 2016 00:04

jseyfried force-pushed the no_per_ns branch 4 times, most recently from 043c4fd to 7f3f96aCompare January 15, 2016 09:03

jseyfried force-pushed the no_per_ns branch from 7f3f96a to d1b5860Compare January 16, 2016 00:00

jseyfried force-pushed the no_per_ns branch from d1b5860 to 866c127Compare January 21, 2016 08:31

jseyfried force-pushed the no_per_ns branch 5 times, most recently from 708e175 to 6865bffCompare January 22, 2016 11:36

jseyfried force-pushed the no_per_ns branch from 6865bff to 521aaf2Compare January 26, 2016 05:35

jseyfried force-pushed the no_per_ns branch 2 times, most recently from ab53df5 to 100a9f6Compare January 28, 2016 03:06

jseyfried added 4 commits January 29, 2016 21:18

Refactor away NameBindings, NsDef, ImportResolutionPerNamespace, Dupl…
118c93b
…icateCheckingMode, and NamespaceDefinition.

Clean up resolve_single_import
e13a045

Nits and other local improvements in resolve
1ca9f03

Remove resolve::dump_module
afd42d2

jseyfried force-pushed the no_per_ns branch from 100a9f6 to afd42d2Compare January 29, 2016 21:26

bors merged commit afd42d2 into rust-lang:masterJan 30, 2016

bors mentioned this pull request Jan 30, 2016
resolve: Finish fixing #30159, a bug in import visibility #30866
Merged

petrochenkov mentioned this pull request Jan 30, 2016
resolve: Get rid of try_define#31308
Closed

jseyfried mentioned this pull request Feb 1, 2016
Items shadow glob-imported reexports #31337
Closed

Uh oh!

Uh oh!

Conversation

jseyfried commented Jan 12, 2016

Uh oh!

jseyfried commented Jan 13, 2016

Uh oh!

bors commented Jan 14, 2016

Uh oh!

bors commented Jan 15, 2016

Uh oh!

nikomatsakis commented Jan 15, 2016

Uh oh!

bors commented Jan 21, 2016

Uh oh!

bors commented Jan 22, 2016

Uh oh!

jseyfried commented Jan 24, 2016

Uh oh!

nrc commented Jan 25, 2016

Uh oh!

nikomatsakis commented Jan 25, 2016

Uh oh!

petrochenkov commented Jan 26, 2016

Uh oh!

jseyfried commented Jan 27, 2016

Uh oh!

petrochenkov commented Jan 27, 2016

Uh oh!

nikomatsakis commented Jan 28, 2016

Uh oh!

petrochenkov commented Jan 28, 2016

Uh oh!

bors commented Jan 29, 2016

Uh oh!

nikomatsakis commented Jan 29, 2016

Uh oh!

jseyfried commented Jan 29, 2016

Uh oh!

nikomatsakis commented Jan 30, 2016

Uh oh!

nikomatsakis commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

alexcrichton commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

alexcrichton commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

bors commented Jan 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants