Skip to content

Assign def ids and build the module graph during expansion - #36601

Merged
bors merged 7 commits into
rust-lang:masterfrom
jseyfried:build_reduced_graph_in_expansion
Sep 28, 2016
Merged

Assign def ids and build the module graph during expansion#36601
bors merged 7 commits into
rust-lang:masterfrom
jseyfried:build_reduced_graph_in_expansion

Conversation

@jseyfried

@jseyfriedjseyfried commented Sep 20, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Groundwork for macro modularization (cc #35896).
r? @nrc

petrochenkov
petrochenkov reviewed Sep 20, 2016
View reviewed changes
Comment threadsrc/librustc_resolve/build_reduced_graph.rs Outdated

@petrochenkovpetrochenkovSep 20, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

macro_escape is ancient! (5bf385b)
Can be removed now.

@jseyfriedjseyfriedSep 20, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

c.f. #36603

@petrochenkovpetrochenkovSep 20, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ugh, I haven't noticed it's span_warn and not span_err, even the variable is named err...

@nrc

nrc commented Sep 20, 2016

Copy link
Copy Markdown
Member

r = me, once the deps land.

@bors

bors commented Sep 22, 2016

Copy link
Copy Markdown
Collaborator

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

@jseyfried
jseyfriedforce-pushed the build_reduced_graph_in_expansion branch 7 times, most recently from 08f131e to 105cc1eCompareSeptember 24, 2016 03:59
jseyfried
jseyfried commented Sep 24, 2016
View reviewed changes
Comment threadsrc/test/compile-fail/gated-non-ascii-idents.rs Outdated

@jseyfriedjseyfriedSep 24, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

After this PR, we load extern crates (in BuildReducedGraphVisitor) before checking for non-ascii idents (in the gated feature checking pass).

@jseyfried

jseyfried commented Sep 24, 2016

Copy link
Copy Markdown
ContributorAuthor

@nrc I added three more commits -- r?

@jseyfried
jseyfriedforce-pushed the build_reduced_graph_in_expansion branch 5 times, most recently from 1dff614 to 0594531CompareSeptember 26, 2016 05:25
nrc
nrc reviewed Sep 26, 2016
View reviewed changes
Comment threadsrc/librustc/hir/map/def_collector.rs Outdated

@nrcnrcSep 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was going to say it would be better to have a struct than a tuple at this point (esp with a bool with comment). However, given that this field only ever seems to have one instantiation, it might just be over-abstracted. Could it be changed to a flag with a method rather than an optional closure?

@jseyfriedjseyfriedSep 26, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I could refactor the closure into a user-defined trait object (e.g. Option<&'a mut MacroInvocationVisitor>, where MacroInvocationVisitor is a trait with method fn visit_invoc(id: NodeId, def_index: DefIndex, const_integer: bool)).

The invocation visiting code needs to be in resolve, so we need a trait object of some sort.

@nrcnrcSep 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a closure is as good as a trait object, but probably better to take a struct rather than a 3-tuple.

@jseyfriedjseyfriedSep 26, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good point -- amended accordingly.

nrc
nrc reviewed Sep 26, 2016
View reviewed changes
Comment threadsrc/librustc_resolve/macros.rs Outdated

@nrcnrcSep 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining what const_integer means please?

@jseyfriedjseyfriedSep 26, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

@jseyfried
jseyfriedforce-pushed the build_reduced_graph_in_expansion branch 2 times, most recently from a21377c to 3aa46c3CompareSeptember 26, 2016 23:27
@bors

bors commented Sep 27, 2016

Copy link
Copy Markdown
Collaborator

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

@jseyfried
jseyfriedforce-pushed the build_reduced_graph_in_expansion branch from 3aa46c3 to dfa69beCompareSeptember 27, 2016 06:44
@jseyfriedjseyfried mentioned this pull request Sep 27, 2016
Merged
@nrc

nrc commented Sep 27, 2016

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented Sep 27, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit dfa69be has been approved by nrc

@bors

bors commented Sep 27, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit dfa69be with merge a059cb2...

bors added a commit that referenced this pull request Sep 27, 2016
@bors
Auto merge of#36601- jseyfried:build_reduced_graph_in_expansion, r=nrc
a059cb2
Assign def ids and build the module graph during expansion
r? @nrc
@bors

bors commented Sep 28, 2016

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: nrc
Pushing a059cb2 to master...

@bors
bors merged commit dfa69be into rust-lang:masterSep 28, 2016
This was referenced Sep 28, 2016
Merged
Closed
This was referenced Sep 29, 2016
Closed
Closed
@jseyfried
jseyfried deleted the build_reduced_graph_in_expansion branch October 16, 2016 09:24
@jseyfriedjseyfried mentioned this pull request Feb 7, 2017
Closed
@jseyfriedjseyfried mentioned this pull request Mar 5, 2017
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkovpetrochenkovpetrochenkov left review comments
+1 more reviewer
@nrcnrcnrc left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@nrcnrc

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jseyfried@nrc@bors@petrochenkov