Skip to content

Override <RangeInclusive as Iterator>::try_(r)fold #56563

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 2 commits into from

Conversation

sinkuu
Copy link
Contributor

@sinkuu sinkuu commented Dec 6, 2018

cc #45222

  • Here preexists a test for folding inclusive ranges.
  • try_rfold for Range is left unoverrided since I didn't see performance gain for it.

Benchmarks:

 name                           orig ns/iter  override ns/iter  diff ns/iter   diff %  speedup 
 bench_triples_inclusive_range  41,481        28,547                 -12,934  -31.18%   x 1.45 
 bench_triples_range            25,245        21,600                  -3,645  -14.44%   x 1.17

@rust-highfive
Copy link
Contributor

r? @withoutboats

(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 Dec 6, 2018
@sinkuu sinkuu force-pushed the range_inclusive_fold branch from fc9de85 to 779066e Compare December 7, 2018 01:44
@sinkuu sinkuu force-pushed the range_inclusive_fold branch from 779066e to 46f6915 Compare December 7, 2018 02:18
@scottmcm
Copy link
Member

scottmcm commented Dec 15, 2018

Shame that this is needed; I thought that the point of #51622 is that it wouldn't be.

Edit: In fact, I'd added this override in #48012, and that PR removed it 😞

cc @kennytm: Do you know anything that might have changed to make LLVM need this again?

@kennytm
Copy link
Member

kennytm commented Dec 15, 2018

@scottmcm No idea 😐

@Centril
Copy link
Contributor

Centril commented Dec 24, 2018

r? @scottmcm

@bors delegate=scottmcm

@bors
Copy link
Collaborator

bors commented Dec 24, 2018

✌️ @scottmcm can now approve this pull request

@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2019

Cross-reference: #57378

@scottmcm
Copy link
Member

r? @rkruppe

Can you take this one too, please, since you have the other RangeInclusive perf one? I think it's better that someone actually on libs handles this, and I'm biased here.

@hanna-kruppe
Copy link
Contributor

Sorry, it doesn't seem like I'll be able to give this PR proper attention in the near future. Please assign someone else.

PS: not sure if that's what you are saying, but I'm not on T-libs or even "affiliated".

@Centril
Copy link
Contributor

Centril commented Jan 22, 2019

Ok... musical chairs... The luck has come to r? @Kimundi

@sinkuu sinkuu closed this Feb 3, 2019
# for free to join this conversation on GitHub. Already have an account? # 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.

9 participants