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 internal iteration in the Sum and Product impls of Result and Option #62459

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@timvermeulen
Copy link
Contributor

commented Jul 7, 2019

This PR adds internal iteration to the ResultShunt iterator type underlying the Sum and Product impls of Result. I had to change ResultShunt to hold a mutable reference to an error instead, similar to itertools::ProcessResults, in order to be able to pass the ResultShunt itself by value (which is necessary for internal iteration).

ResultShunt::process can unfortunately no longer be an associated function because that would make it generic over the lifetime of the error reference, which wouldn't work, so I turned it into the free function process_results.

I removed the OptionShunt type and forwarded the Sum and Product impls of Option to their respective impls of Result instead, to avoid having to repeat the internal iteration logic.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

r? @shepmaster

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

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

@rust-highfive rust-highfive assigned scottmcm and unassigned shepmaster Jul 7, 2019

@timvermeulen timvermeulen force-pushed the timvermeulen:result_sum_internal_iteration branch from 14e49b9 to 1f1904a Jul 7, 2019

@Stargateur

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

look, like we are in conflict #62883

I tried to keep OptionShunt while delegate the implementation to ResultShunt do you think it's possible to make it compile #62883 (comment) ?

Also, are you sure your code doesn't slow done thing ? Why should take a reference to an error should be better ?

@shepmaster

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Ping @scottmcm — probably worth reviewing both related PRs.

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@Stargateur So it looks like my PR removes the duplication between OptionShunt and ResultShunt, while yours removes the duplication between those types and the two Adapter types used in the FromIterator impls? It seems to me like our PRs are totally compatible. If your PR is merged first, then I can still apply these changes to ResultShunt / OptionShunt, and I could implement Option::from_iter in terms of Result::from_iter. How does that sound?

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

@timvermeulen Sounds like a good plan. I approved #62883, and will wait on your update here.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

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

@timvermeulen timvermeulen force-pushed the timvermeulen:result_sum_internal_iteration branch from 1f1904a to 2e41ba8 Jul 29, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@scottmcm I updated my branch, it should now match what I described in my previous comment 🙂

@edmilsonefs

This comment has been minimized.

Copy link

commented Aug 6, 2019

Hey! This is a ping from triage, we would like to know if you @scottmcm could give us a few more minutes to update us from last changes made by @timbertson .

Thanks.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Very nice! Great to see the .by_ref() disappearing 🎉

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

📌 Commit 2e41ba8 has been approved by scottmcm

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

⌛️ Testing commit 2e41ba8 with merge c0832e4...

bors added a commit that referenced this pull request Aug 6, 2019
Auto merge of #62459 - timvermeulen:result_sum_internal_iteration, r=…
…scottmcm

Use internal iteration in the Sum and Product impls of Result and Option

This PR adds internal iteration to the `ResultShunt` iterator type underlying the `Sum` and `Product` impls of `Result`. I had to change `ResultShunt` to hold a mutable reference to an error instead, similar to `itertools::ProcessResults`, in order to be able to pass the `ResultShunt` itself by value (which is necessary for internal iteration).

`ResultShunt::process` can unfortunately no longer be an associated function because that would make it generic over the lifetime of the error reference, which wouldn't work, so I turned it into the free function `process_results`.

I removed the `OptionShunt` type and forwarded the `Sum` and `Product` impls of `Option` to their respective impls of `Result` instead, to avoid having to repeat the internal iteration logic.
Centril added a commit to Centril/rust that referenced this pull request Aug 6, 2019
Rollup merge of rust-lang#62459 - timvermeulen:result_sum_internal_it…
…eration, r=scottmcm

Use internal iteration in the Sum and Product impls of Result and Option

This PR adds internal iteration to the `ResultShunt` iterator type underlying the `Sum` and `Product` impls of `Result`. I had to change `ResultShunt` to hold a mutable reference to an error instead, similar to `itertools::ProcessResults`, in order to be able to pass the `ResultShunt` itself by value (which is necessary for internal iteration).

`ResultShunt::process` can unfortunately no longer be an associated function because that would make it generic over the lifetime of the error reference, which wouldn't work, so I turned it into the free function `process_results`.

I removed the `OptionShunt` type and forwarded the `Sum` and `Product` impls of `Option` to their respective impls of `Result` instead, to avoid having to repeat the internal iteration logic.
@Centril Centril referenced this pull request Aug 6, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Aug 6, 2019
Auto merge of #63325 - Centril:rollup-gaqv2nd, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #62459 (Use internal iteration in the Sum and Product impls of Result and Option)
 - #62837 (Fix theme picker blur handler: always hide instead of switching)
 - #63152 (Always error on `SizeOverflow` during mir evaluation)
 - #63286 (Replace error callback with Result)
 - #63298 (assume_init: warn about valid != safe)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

⌛️ Testing commit 2e41ba8 with merge 320ac87...

bors added a commit that referenced this pull request Aug 6, 2019
Auto merge of #62459 - timvermeulen:result_sum_internal_iteration, r=…
…scottmcm

Use internal iteration in the Sum and Product impls of Result and Option

This PR adds internal iteration to the `ResultShunt` iterator type underlying the `Sum` and `Product` impls of `Result`. I had to change `ResultShunt` to hold a mutable reference to an error instead, similar to `itertools::ProcessResults`, in order to be able to pass the `ResultShunt` itself by value (which is necessary for internal iteration).

`ResultShunt::process` can unfortunately no longer be an associated function because that would make it generic over the lifetime of the error reference, which wouldn't work, so I turned it into the free function `process_results`.

I removed the `OptionShunt` type and forwarded the `Sum` and `Product` impls of `Option` to their respective impls of `Result` instead, to avoid having to repeat the internal iteration logic.
Centril added a commit to Centril/rust that referenced this pull request Aug 6, 2019
Rollup merge of rust-lang#62459 - timvermeulen:result_sum_internal_it…
…eration, r=scottmcm

Use internal iteration in the Sum and Product impls of Result and Option

This PR adds internal iteration to the `ResultShunt` iterator type underlying the `Sum` and `Product` impls of `Result`. I had to change `ResultShunt` to hold a mutable reference to an error instead, similar to `itertools::ProcessResults`, in order to be able to pass the `ResultShunt` itself by value (which is necessary for internal iteration).

`ResultShunt::process` can unfortunately no longer be an associated function because that would make it generic over the lifetime of the error reference, which wouldn't work, so I turned it into the free function `process_results`.

I removed the `OptionShunt` type and forwarded the `Sum` and `Product` impls of `Option` to their respective impls of `Result` instead, to avoid having to repeat the internal iteration logic.
@Centril Centril referenced this pull request Aug 6, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@bors retry

bors added a commit that referenced this pull request Aug 6, 2019
Auto merge of #63328 - Centril:rollup-482ujaf, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #62459 (Use internal iteration in the Sum and Product impls of Result and Option)
 - #62821 (Not listed methods)
 - #62837 (Fix theme picker blur handler: always hide instead of switching)
 - #63286 (Replace error callback with Result)
 - #63296 (Deduplicate rustc_demangle in librustc_codegen_llvm)
 - #63298 (assume_init: warn about valid != safe)

Failed merges:

r? @ghost

@bors bors merged commit 2e41ba8 into rust-lang:master Aug 6, 2019

4 of 5 checks passed

homu Testing commit 2e41ba8742c341eaf1ec1b5954e089e4d2c02dd0 with merge 320ac879e9127dea1139438ed82ca841b89c4954...
Details
pr Build #20190729.3 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.