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

Use the macro structure spans instead of the invocation #43352

Merged
merged 3 commits into from
Jul 22, 2017

Conversation

estebank
Copy link
Contributor

Fix #42104, CC #2887.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@estebank estebank force-pushed the macro-span-replacement branch 2 times, most recently from 82a3d26 to db43a93 Compare July 20, 2017 16:14
@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2017
@estebank
Copy link
Contributor Author

r? @petrochenkov

@@ -184,15 +185,32 @@ impl Span {
result
}

pub fn empty_ctxt(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This new function is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

self.hi
} else {
end.hi
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to use standard min and max for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -241,6 +258,21 @@ impl TokenStream {
}
}

pub fn map_pos<F: FnMut(usize, TokenTree) -> TokenTree>(self, mut f: F) -> TokenStream {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this into enumerate_map (or map_enumerated), to use the same naming conventions as Iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// rhs has holes ( `$id` and `$(...)` that need filled)
let tts = transcribe(cx, Some(named_matches), rhs);
let mut tts = transcribe(cx, Some(named_matches), rhs.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure, everything is under Rc and cloning/mapping is cheap, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All I need is the spans after this, so I'm cloning the spans first into their own Vec.

@petrochenkov
Copy link
Contributor

Reviewed.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 21, 2017

📌 Commit 6772661 has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Jul 22, 2017

⌛ Testing commit 6772661 with merge 35f6499...

bors added a commit that referenced this pull request Jul 22, 2017
Use the macro structure spans instead of the invocation

Fix #42104, CC #2887.
@bors
Copy link
Contributor

bors commented Jul 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 35f6499 to master...

@bors bors merged commit 6772661 into rust-lang:master Jul 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants