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

Remove unused methods from MultiSpan #34965

Merged
merged 1 commit into from
Jul 24, 2016
Merged

Conversation

sophiajt
Copy link
Contributor

Removed a couple of unused methods from MultiSpan. I thought about batching this with some other changes but wasn't sure when I'd get around to them, so PR for a tiny fix instead.

This can be rollup'd.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@sophiajt
Copy link
Contributor Author

@bors r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned aturon Jul 21, 2016
@sanxiyn
Copy link
Member

sanxiyn commented Jul 22, 2016

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 22, 2016

📌 Commit f7019a4 has been approved by sanxiyn

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 24, 2016
…r=sanxiyn

Remove unused methods from MultiSpan

Removed a couple of unused methods from MultiSpan.  I thought about batching this with some other changes but wasn't sure when I'd get around to them, so PR for a tiny fix instead.

This can be rollup'd.
bors added a commit that referenced this pull request Jul 24, 2016
Rollup of 7 pull requests

- Successful merges: #34965, #34972, #34975, #34976, #34977, #34988, #34989
- Failed merges:
@bors bors merged commit f7019a4 into rust-lang:master Jul 24, 2016
@mcarton
Copy link
Member

mcarton commented Jul 26, 2016

This PR seems to have removed the only way to create a MultiSpan with more that one primary span (the only two ways to create one are now MultiSpan::new() and impl From<Span> for MultiSpan). Since primary_spans is a vector and used everywhere as if there can be more than one primary span (it's sorted, its min is taken, etc) I wonder whether this is intentional. If so, more cleanup should be done and primary_spans should be made an option.

This also breaks Clippy (https://github.com/Manishearth/rust-clippy/issues/1120, https://github.com/Manishearth/rust-clippy/issues/1119) without a simple way to fix it, so I'd like to know whether I can hope getting that method back or should remove the broken Clippy feature.

@sophiajt
Copy link
Contributor Author

@mcarton I joined the discussion on https://github.com/Manishearth/rust-clippy/pull/1119 to see if there are alternate fixes for clippy. Since MultiSpan isn't being used in the compiler in this way, I wanted to continue to simplify it. As you point out, there's more there to be done.

@sophiajt sophiajt deleted the multispan_cleanup branch August 18, 2016 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants