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.

Sign up for GitHub

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

Jump to bottom

add fn into_inner(self) -> (Idx, Idx) to RangeInclusive (#49022) #50574

Merged
merged 1 commit into from May 11, 2018
Merged

add fn into_inner(self) -> (Idx, Idx) to RangeInclusive (#49022) #50574

merged 1 commit into from May 11, 2018

Conversation

ghost
Copy link

@ghost ghost commented May 9, 2018

adds into_inner(self) -> (Idx, Idx) to RangeInclusive
#49022 (comment)

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TimNN (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 9, 2018
@ghost ghost mentioned this pull request May 9, 2018
Closed
3 tasks
@SimonSapin
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 9, 2018

📌 Commit 23aa483 has been approved by SimonSapin

@bors bors 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 May 9, 2018
@SimonSapin
Copy link
Contributor

Hmm, or maybe into_start_end? What do you think? @s3bk

@ghost
Copy link
Author

ghost commented May 9, 2018 •
edited by ghost
Loading

@SimonSapin into_inner is a name that is used all over the place in std for this purpose. So I think it makes sense, even when there are just two fields.

@SimonSapin
Copy link
Contributor

Yeah but isn’t the into_inner precedent for methods that only return one field?

alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 10, 2018
@alexcrichton
Rollup merge of rust-lang#50574 - s3bk:range_inclusive_into_inner, r=…
0989adf 
…SimonSapin

add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (rust-lang#49022)

adds `into_inner(self) -> (Idx, Idx)` to RangeInclusive
rust-lang#49022 (comment)
@alexcrichton alexcrichton mentioned this pull request May 10, 2018
Closed
alexcrichton added a commit to alexcrichton/rust that referenced this pull request May 10, 2018
@alexcrichton
Rollup merge of rust-lang#50574 - s3bk:range_inclusive_into_inner, r=…
445e53e 
…SimonSapin

add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (rust-lang#49022)

adds `into_inner(self) -> (Idx, Idx)` to RangeInclusive
rust-lang#49022 (comment)
@alexcrichton alexcrichton mentioned this pull request May 10, 2018
Merged
bors added a commit that referenced this pull request May 10, 2018
@bors
Auto merge of #50611 - alexcrichton:rollup, r=alexcrichton
a006328 
Rollup of 18 pull requests

Successful merges:

 - #49423 (Extend tests for RFC1598 (GAT))
 - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check))
 - #50447 (Fix update-references for tests within subdirectories.)
 - #50514 (Pull in a wasm fix from LLVM upstream)
 - #50524 (Make DepGraph::previous_work_products immutable)
 - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.)
 - #50538 ( Make CrateNum allocation more thread-safe. )
 - #50564 (Inline `Span` methods.)
 - #50565 (Use SmallVec for DepNodeIndex within dep_graph.)
 - #50569 (Allow for specifying a linker plugin for cross-language LTO)
 - #50572 (Clarify in the docs that `mul_add` is not always faster.)
 - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022))
 - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`)
 - #50588 (Move "See also" disambiguation links for primitive types to top)
 - #50590 (Fix tuple struct field spans)
 - #50591 (Restore RawVec::reserve* documentation)
 - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize)
 - #50606 (Retry when downloading the Docker cache.)

Failed merges:

 - #50161 (added missing implementation hint)
 - #50558 (Remove all reference to DepGraph::work_products)
@bors bors merged commit 23aa483 into rust-lang:master May 11, 2018
@ghost ghost deleted the range_inclusive_into_inner branch May 11, 2018 09:08
@smmalis37
Copy link
Contributor

Should this be added to other Range* types for consistency? Or could it be added to the RangeBounds trait?

@ghost
Copy link
Author

ghost commented May 16, 2018

@smmalis37: Probably a good idea. But the return type changes, so it would not fit in the RangeBounds trait. Maybe a fn into_bounds(self) -> (Bound<Index>, Bound<Index>) would work?

RangeFull::into_inner(self) -> () ?
RangeTo::into_inner(self) -> Index
RangeFrom::into_inner(self) -> Index
Range::into_inner(self) -> (Index, Index)
RangeInclusive::into_inner(self) -> (Index, Index)

@SimonSapin
Copy link
Contributor

The fields of other range types are public, so you can directly reference them or move out of them. And I’m skeptical that into_inner is useful in a generic context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@TimNN TimNN

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rust-highfive @SimonSapin @bors @smmalis37 @TimNN