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

Add Eval documentation #1816

Merged
merged 4 commits into from
Sep 20, 2017
Merged

Add Eval documentation #1816

merged 4 commits into from
Sep 20, 2017

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Aug 11, 2017

Adds some eval documentation with very basic examples, not sure if there are better examples around that can better showcase the usefulness of Eval. Either way, looking forward to feedback :)

@LukaJCB LukaJCB mentioned this pull request Aug 11, 2017
70 tasks
@codecov-io
Copy link

codecov-io commented Aug 11, 2017

Codecov Report

Merging #1816 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1816      +/-   ##
==========================================
+ Coverage   94.88%   94.96%   +0.07%     
==========================================
  Files         241      241              
  Lines        4148     4173      +25     
  Branches      102      106       +4     
==========================================
+ Hits         3936     3963      +27     
+ Misses        212      210       -2
Impacted Files Coverage Δ
laws/src/main/scala/cats/laws/ApplyLaws.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Ior.scala 100% <0%> (ø) ⬆️
...c/main/scala/cats/laws/discipline/ApplyTests.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Tuple2K.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/EitherT.scala 98.27% <0%> (ø) ⬆️
core/src/main/scala/cats/data/WriterT.scala 94.18% <0%> (+0.13%) ⬆️
core/src/main/scala/cats/data/IdT.scala 97.29% <0%> (+0.15%) ⬆️
core/src/main/scala/cats/instances/stream.scala 96.42% <0%> (+0.27%) ⬆️
core/src/main/scala/cats/instances/map.scala 94.11% <0%> (+0.56%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e7175...f84cfa8. Read the comment docs.

@johnynek
Copy link
Contributor

This does not seem to mention that some uses of nested .value calls can be unsafe and kill stack safety.

Ideally, you never call .value inside something that could be itself inside an Eval. I don't fully grok, myself rules for when it might be safe to call .value inside another Eval, but @non seems to have some rules of thumb. I think without this discussion we could be setting someone up for confusion stack overflows.

@johnynek
Copy link
Contributor

note for instance #1703 which I think may be currently broken still.

@johnynek
Copy link
Contributor

Can we add an example of using Eval.defer to do stack safe recursion? That's one of the main motivators of Eval (along with giving a type to an optionally lazy value).

another note: it would be really nice to have some, even non-rigorous, benchmarks on Eval. In fact, Eval is MUCH slower than using standard function calls. So if you are doing it to get stack safe recursion, if you know the recursion depth will be shallow, you may be much better off with normal on-stack recursion.

cc @non


#### Eval.later

If we want lazy evaluation, we can use `Eval.later`:
Copy link
Contributor

@kailuowang kailuowang Aug 11, 2017

Choose a reason for hiding this comment

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

Might be worthy mentioning here that there are two advantages of Later over lazy val

  1. Later allows the thunk being GC'd after evaluation and
  2. Later's synchronization lock locks itself while lazy vals lock their enclosing class. Discussed in detail here

@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Aug 11, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-RC1 Aug 29, 2017
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Thanks @LukaJCB!

I think that @johnynek and @kailuowang have raised some good points, but I also think that having this documentation is much better than having none for Eval, and I think that they could be addressed in followup PRs.

@kailuowang
Copy link
Contributor

I agree with @ceedubs that having this is much better than having none, @LukaJCB what do you think? Do you think those points are super quick to address or do you want to iterate fast?
BTW, there is an awesome talk by @non on this topic. https://www.youtube.com/watch?v=XDDy-QD0aVU maybe worth mentioning

@LukaJCB
Copy link
Member Author

LukaJCB commented Sep 12, 2017

I did address @kailuowangs points and I also added a small note on Eval.defer :)
I was going to wait for @non to chime in here, about stack-safe recursion, since I'm not super knowledgable on that, but I'm fine with merging now, and making a new PR with Eval.defer.

@ceedubs
Copy link
Contributor

ceedubs commented Sep 20, 2017

Merging with 2 approvals. I do think that @kailuowang's suggestion to link to Erik's talk on Eval would be a nice addition in a future PR.

@ceedubs ceedubs merged commit 5a3d1eb into typelevel:master Sep 20, 2017
@LukaJCB LukaJCB deleted the add-eval-docs branch September 20, 2017 15:57
@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017
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.

5 participants