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

Make last structs indexes definitions use newtype_index macro #45770

Merged
merged 3 commits into from
Nov 6, 2017

Conversation

spastorino
Copy link
Member

This PR makes the last two index structs not using newtype_index macro to use it and also fixes this #45763 issue.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@spastorino
Copy link
Member Author

/cc @nikomatsakis @Nashenas88

@spastorino
Copy link
Member Author

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Nov 4, 2017
const INVALID: DepNodeIndex = DepNodeIndex {
index: ::std::u32::MAX,
};
const INVALID: DepNodeIndex = DepNodeIndex(::std::u32::MAX);
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this one isn't in the macro call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need graph::DepNodeIndex::INVALID and the macro generates graph::INVALID

@@ -157,7 +157,7 @@ macro_rules! newtype_index {
(@pub [$($pub:tt)*]
Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note, this could be using $pub:vis instead of $($pub:tt)*.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could add that as a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

My attempt to make this change https://gist.github.com/spastorino/715079ea4839effe67fd9f0c3bee1c70

And I'm getting a lot of these errors ...

error: no rules expected the token `]`
  --> src/librustc/dep_graph/graph.rs:48:1
   |
48 | newtype_index!(DepNodeIndex);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro outside of the current crate

Ran with trace macros ...

note: trace_macro
  --> src/librustc/dep_graph/graph.rs:48:1
   |
48 | newtype_index!(DepNodeIndex);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: expanding `newtype_index! { DepNodeIndex }`
   = note: to `newtype_index ! (
           @ type [ DepNodeIndex ] @ max [ :: std :: u32 :: MAX ] @ debug_format [ "{}" ]
           ) ;`
   = note: expanding `newtype_index! { @ type [ DepNodeIndex ] @ max [ :: std :: u32 :: MAX ] @ debug_format [ "{}" ] }`
   = note: to `newtype_index ! (
           @ vis [  ] @ type [ DepNodeIndex ] @ max [ ::std::u32::MAX ] @ debug_format [
           "{}" ] ) ;`
   = note: expanding `newtype_index! { @ vis [  ] @ type [ DepNodeIndex ] @ max [ ::std::u32::MAX ] @ debug_format [
           "{}" ] }`

error: no rules expected the token `]`
  --> src/librustc/dep_graph/serialized.rs:17:1
   |
17 | newtype_index!(SerializedDepNodeIndex);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro outside of the current crate

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be #41949. Probably not worth it to fix in this PR :)

@spastorino
Copy link
Member Author

@nikomatsakis I'd merge this if you are ok and then this vis thing could be done in a separate PR.

Copy link
Contributor

@Nashenas88 Nashenas88 left a comment

Choose a reason for hiding this comment

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

Nice work!

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2017
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 5, 2017

📌 Commit d19faea has been approved by nikomatsakis

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2017
@bors
Copy link
Contributor

bors commented Nov 5, 2017

⌛ Testing commit d19faea with merge 44990e5...

bors added a commit that referenced this pull request Nov 5, 2017
Make last structs indexes definitions use newtype_index macro

This PR makes the last two index structs not using newtype_index macro to use it and also fixes this #45763 issue.
@bors
Copy link
Contributor

bors commented Nov 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 44990e5 to master...

@bors bors merged commit d19faea into rust-lang:master Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants