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

Improve AndThen use of Single #3560

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Conversation

johnynek
Copy link
Contributor

@johnynek johnynek commented Aug 10, 2020

follow up to #3527

Here we still do O(1) work on andThen and compose, but we check improve in two ways:

  1. we check to see if we have a Concat(_, Single(_, _)) that we can extend. Otherwise the first Concat will break the andThen chaining up to fusionMaxStackDepth.
  2. we check to see if the andThen/compose arguments are AndThen instances early. This is a safety fix, otherwise Concat(_, ).andThen(Concat(, _)) puts the right concat inside a single apply call in the loop, which could blow the stack since inside there could also do the same.

Note, in the fallback we were already testing for _: AndThen, here we inline that test to always happen early so we can leverage that information.

@codecov-commenter
Copy link

Codecov Report

Merging #3560 into master will decrease coverage by 0.05%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master    #3560      +/-   ##
==========================================
- Coverage   91.29%   91.23%   -0.06%     
==========================================
  Files         386      386              
  Lines        8577     8589      +12     
  Branches      240      252      +12     
==========================================
+ Hits         7830     7836       +6     
- Misses        747      753       +6     

Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me.

@PeterPerhac
Copy link
Contributor



@johnynek johnynek merged commit 330ac25 into master Aug 13, 2020
@travisbrown travisbrown added this to the 2.2.0-RC3 milestone Aug 15, 2020
@larsrh larsrh deleted the oscar/right_associate_more_andthen branch September 19, 2020 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants