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 bound-fn in let-flow #202

Merged
merged 4 commits into from
Sep 6, 2021

Conversation

tanzoniteblack
Copy link
Contributor

bound-fn preserves bindings of dynamic variables, even if the function ends up
being executed on a different thread.

I've made sure to test that without this change, the test I've added to detect
dynamic variables across executor threads actually does still fail.

`bound-fn` preserves bindings of dynamic variables, even if the function ends up
being executed on a different thread.

I've made sure to test that without this change, the test I've added to detect
dynamic variables across executor threads actually does still fail.
@tanzoniteblack
Copy link
Contributor Author

Backport of yummly#2 , which I briefly mentioned in #183

The promise/delay code used in Dirigiste is to avoid creating the pools
until/unless they're used, iiuc. Since we know we need an Executor for
the test, we can create it immediately.
@KingMob
Copy link
Collaborator

KingMob commented Sep 5, 2021

@tanzoniteblack I recently updated Manifold's indentation, partly to fix some inconsistencies, partly to modernize it with current styles. This caused some conflicts, which I fixed.

As part of that, I also took a closer look at the new test for bound-fn in let-flow. If I'm not mistaken, the code to create the executor was taken from how manifold.executor sets up its default pools. manifold.executor uses a complicated delay/promise setup to avoid creating the executors until/unless they're actually used, but since we know we need an executor for the test, we can create it directly.

I went ahead and simplified the test code, as well as verifying the test results were the same, both before and after the bound-fn change, but let me know if you think I missed something.

@tanzoniteblack
Copy link
Contributor Author

Looks good, @KingMob

@KingMob KingMob merged commit f32f4e9 into clj-commons:master Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants