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

Introduce RangeInclusive::{new, start, end} methods and make the fields private. #49724

Merged
merged 3 commits into from
May 1, 2018
Merged

Introduce RangeInclusive::{new, start, end} methods and make the fields private. #49724

merged 3 commits into from
May 1, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Apr 6, 2018 •
edited
Loading

cc #49022

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 6, 2018
@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2018
@kennytm kennytm removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2018
@petrochenkov
Copy link
Contributor

I suspect this can be done without introducing RangeInclusive::new and changing desugaring by setting hygiene context for field identifiers as if they come from a macro 2.0 defined in libcore/ops/range.rs.

@kennytm kennytm force-pushed the range-inc-start-end-methods branch 3 times, most recently from d6b7342 to 2415f53 Compare April 6, 2018 14:56
@rust-lang rust-lang deleted a comment from TimNN Apr 6, 2018
@rust-lang rust-lang deleted a comment from TimNN Apr 6, 2018
@rust-lang rust-lang deleted a comment from TimNN Apr 6, 2018
@kennytm kennytm force-pushed the range-inc-start-end-methods branch from 2415f53 to 8ea4d30 Compare April 6, 2018 16:51
@rust-lang rust-lang deleted a comment from TimNN Apr 6, 2018
@kennytm kennytm mentioned this pull request Apr 6, 2018
Closed
3 tasks
@kennytm
Copy link
Member Author

kennytm commented Apr 6, 2018

@petrochenkov That sounds like a huge hack to just avoid adding a new function 😅, and it also ties the lowering to whatever internal representation we will choose in the future. There's also support of adding a constructor function as well.

@kennytm kennytm force-pushed the range-inc-start-end-methods branch from 8ea4d30 to b7b82d3 Compare April 6, 2018 17:18
@rust-lang rust-lang deleted a comment from TimNN Apr 6, 2018
@bors
Copy link
Contributor

bors commented Apr 6, 2018

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

@kennytm kennytm force-pushed the range-inc-start-end-methods branch from b7b82d3 to def13dd Compare April 6, 2018 19:17
@rust-lang rust-lang deleted a comment from TimNN Apr 6, 2018
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2018
@kennytm kennytm changed the title [WIP] Introduce RangeInclusive::{new, start, end} methods and make the fields private. Introduce RangeInclusive::{new, start, end} methods and make the fields private. Apr 7, 2018
@kennytm kennytm added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2018
@kennytm
Copy link
Member Author

kennytm commented Apr 7, 2018

(Blocked by FCP in #49022)

@scottmcm
Copy link
Member

scottmcm commented Apr 7, 2018

One additional thing this stabilizes that might be worth considering: The post-iteration values of start and end. (One nice thing about "just use next() and last()" was that they returned None in that case.)

The docs say not to rely on it, but right now it goes to 1..=0, which would potentially want to change if one of the other alternatives showed up.

@kennytm
Copy link
Member Author

kennytm commented Apr 7, 2018

@scottmcm Added docs to say the values are unspecified.

I expect we are going to abandon RFC 1980 since the representation is now a private detail, in favor of performance. There will likely be a boolean field representing whether iteration has been completed. 1..=4 will end up with start=5, end=4, is_empty=true, and 250..=255 will end up with start=0, end=255, is_empty=true.

@scottmcm
Copy link
Member

scottmcm commented Apr 7, 2018

@kennytm Yup, that outcome for iteration wouldn't surprise me at all. (It can't be done with today's Step, of course, but that's not a blocker.) Just wanted to make sure it was noted, and we have a good case if people complain that their assert_eq!(foo, 1..=0); tests don't work any more after the change.

@kennytm kennytm added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Apr 17, 2018
@kennytm
Copy link
Member Author

kennytm commented Apr 20, 2018

(FCP of #49022 has completed)

@bors
Copy link
Contributor

bors commented Apr 22, 2018

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

@pietroalbini
Copy link
Member

Ping from triage @Kimundi! This PR needs your review!
Also @kennytm, there are conflicts that needs to be resolved.

@kennytm
Make the fields of RangeInclusive private.
fba903a 
Added new()/start()/end() methods to RangeInclusive.

Changed the lowering of `..=` to use RangeInclusive::new().
@kennytm kennytm force-pushed the range-inc-start-end-methods branch from e3dbebc to 8379deb Compare April 30, 2018 13:06
@rust-highfive

This comment has been minimized.

Sign in to view
@kennytm kennytm force-pushed the range-inc-start-end-methods branch from 8379deb to d52ec99 Compare April 30, 2018 15:52
@rust-highfive

This comment has been minimized.

Sign in to view
@kennytm kennytm force-pushed the range-inc-start-end-methods branch 2 times, most recently from 6967c8d to 61fc692 Compare April 30, 2018 17:29
@rust-highfive

This comment has been minimized.

Sign in to view
@kennytm kennytm force-pushed the range-inc-start-end-methods branch from 61fc692 to f70b2eb Compare April 30, 2018 17:45
@kennytm
Copy link
Member Author

kennytm commented May 1, 2018

@bors r=Kimundi

(Approved by #49724 (review))

@bors
Copy link
Contributor

bors commented May 1, 2018

📌 Commit f70b2eb has been approved by Kimundi

@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 1, 2018
@bors
Copy link
Contributor

bors commented May 1, 2018

⌛ Testing commit f70b2eb with merge a4a7947...

bors added a commit that referenced this pull request May 1, 2018
@bors
Auto merge of #49724 - kennytm:range-inc-start-end-methods, r=Kimundi
a4a7947 
Introduce RangeInclusive::{new, start, end} methods and make the fields private.

cc #49022
@bors
Copy link
Contributor

bors commented May 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Kimundi
Pushing a4a7947 to master...

@bors bors merged commit f70b2eb into rust-lang:master May 1, 2018
@kennytm kennytm deleted the range-inc-start-end-methods branch May 1, 2018 14:39
@pietroalbini pietroalbini mentioned this pull request May 3, 2018
Closed
@oli-obk oli-obk mentioned this pull request May 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kimundi Kimundi Kimundi approved these changes

Assignees

@Kimundi Kimundi

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.

None yet
8 participants
@kennytm @rust-highfive @petrochenkov @bors @scottmcm @clarfonthey @pietroalbini @Kimundi