Implement foreign definitions #1
Conversation
|
@anttih is this one good to review? |
|
@f-f not yet. There's too much code that has no test coverage. I'm trying to at least run the test suite locally with no inlining. |
|
This is ready for review. It's quite difficult to tell if we have full test coverage. I did add some tests for arrays. |
| main :: Effect Unit | ||
| main = do | ||
| testNumberShow show | ||
| -- testNumberShow show |
There was a problem hiding this comment.
I left this for another PR. Partly because I didn't know how to implement it yet. Should I somehow properly leave a TODO or make an issue? I wanted comment it out so I could run the test suite.
| assert "degree returns absolute integers" $ degree 4 == 4 | ||
| assert "degree returns absolute integers" $ degree bot >= 0 | ||
| assert "degree does not return out-of-bounds integers" $ degree bot <= top | ||
| -- assert "degree returns absolute integers" $ degree bot >= 0 |
There was a problem hiding this comment.
This one I explain in the PR description. Basically degree bot can overflow, just like any operation on Int. We might be able to solve this one with another implementation of degree, but it's not clear to me how that would help since any operation can overflow anyway.
There was a problem hiding this comment.
Makes sense. Let's leave a comment and a TODO about this so the future implementors will find a thread to follow
2 participants
Add foreign definitions for almost all missing modules.
Noteworthy things:
Int(fixnums). This means that any operation on anIntmight overflow. For exampledegree botwill overflow and was removed from the test suite. I think this is a better compromise than trying to somehow prevent any overflow, which would be very difficult to do anyway.unitis the symbolunit.AlmostEffwas replaced with "normal"Effectby including the definition in the test suite. This change we probably should upstream, but it wasn't clear to me how.Also note that since theIt's now defined asEffectis inUtils.Effectsome of the inlining will not trigger. Not sure if this is good or bad.Effect, and inlining works.doloops to slightly simpler tail-recursive variants.Showmodule whose implementations are probably not fully compatible. Update: now just usingformat. Not sure how much we should worry about theShowinstances for the primitives.The test suite does pass but there is no easy way to run it currently. I will soon post a PR for theNow it can be run, and passes.purs-backend-chezcli tool so that people can try this out.degree botoverflows because(fxabs (most-negative-fixnum))overflows. Maybe we can figure out a different implementation forintDegree? Meanwhile the test is commented out.Checklist: