Skip to content

Conversation

@safareli
Copy link
Contributor

fix #83

Copy link
Contributor Author

@safareli safareli left a comment

Choose a reason for hiding this comment

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

package.json Outdated
"purescript-psa": "^0.5.0-rc.1",
"pulp": "^11.0.x",
"purescript": "0.11.x",
"purescript-psa": "^0.5.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

are this changes fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by adding ps here we don't need to fetch it by hand in build

Copy link
Member

Choose a reason for hiding this comment

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

For the core libraries we prefer to fetch by hand - it means that we can update libraries before the npm release is done.


-- | Suspend a computation by one step.
suspend :: forall a. Trampoline a -> Trampoline a
suspend = suspendF
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand what's the point of suspendF and suspend. does it make sense for this implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove it since this is breaking anyway.

@safareli
Copy link
Contributor Author

btw /benchmark is outdated and i was not able to run it using PS@0.11

- chmod a+x $HOME/purescript
- npm install -g bower
- npm install
script:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the point of two bower Installs here?

Copy link
Member

Choose a reason for hiding this comment

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

The second bower install will fetch devDependencies, splitting the build/test/install stuff this way ensures we're not accidentally relying on dev dependencies in the library.

@ethul
Copy link
Collaborator

ethul commented Apr 25, 2017

This may already be in the works or may have already been decided; however, I am wondering if it would make sense to add a performance comparison to test the note in #83 that the implementation in this PR might be more efficient.

@safareli
Copy link
Contributor Author

safareli commented Apr 26, 2017

@ethul I would need to update /benchmark as it's outdated. will try

so:

  • /benchmark should be update to ps@0.11
  • current Trampoline implementation should be copied there (it should uses current Free)
  • update Trampoline benchmarks to use all three implementations (old, current and next)

@ethul
Copy link
Collaborator

ethul commented Apr 26, 2017 via email

@safareli safareli mentioned this pull request Oct 25, 2017
@natefaubion
Copy link
Contributor

Is the Lazy/thunk even necessary? Why isn't the spine of Free Identity enough to act as a trampoline?

@natefaubion
Copy link
Contributor

@natefaubion
Copy link
Contributor

FWIW, I'm not sure that Free Lazy memoizes in a very interesting way. If you defer under a monadic bind, it won't share anything because it creates a new Lazy every time you apply the bind.

@paf31
Copy link
Contributor

paf31 commented Oct 26, 2017

It's useful to form cyclic structures, and I think some could be generated purely using Lazy which would then share some results. Things like finding comonadic fixed points jump to mind. That might be useful, but whether it's useful enough to add the performance overhead for the simple case, I don't know.

@safareli
Copy link
Contributor Author

What if we just remove it as there are already 3 ways to get trampoline? (Lazy, (->) Unit, Identity)

@paf31
Copy link
Contributor

paf31 commented Oct 27, 2017

Uh, sorry, I was thinking of Cofree, not Trampoline.

@safareli
Copy link
Contributor Author

What if we just remove it as there are already 3 ways to get trampoline? (Lazy, (->) Unit, Identity)

?

@garyb garyb changed the base branch from master to compiler/0.12 May 22, 2018 15:15
@garyb garyb merged commit c3bec77 into purescript:compiler/0.12 May 22, 2018
@safareli safareli deleted the lazytoarr branch May 22, 2018 16:38
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.

5 participants