Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Inject/Project find effects nested on the left #219
Inject/Project find effects nested on the left #219
Changes from 12 commits
72790d5
cdacd2c
51cf026
e90add7
f932ce7
80ab68a
d507e93
c487e10
7581853
f05122c
1eb8195
cd34b45
ca0d6fc
78aff76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically just reassociating here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reassociation here won’t be too bad a performance hit as long as we’re reasonable about using left-nested sums.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on what you mean by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And can we document why we need to reassociate here (and maybe drop a link to DTALC for people who are wondering about why all these OVERLAPPABLE pragmas need to happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, this assumes that trees will generally be right-leaning. So anything exclusively right-chained won’t ever see this instance, and if you have
(A :+: B) :+: C
that’s an extremely minor hit (as your benchmark demonstrates). But a long left-chain—which is extremely unlikely to happen, and never by accident; you would have to work quite hard to design your effects thus—will take more of a hit because it has to do a bunch more reassociation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m gonna do this in #223 to avoid conflicts.