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

Mal implementation by Crystal #70

Merged
merged 31 commits into from
Jun 3, 2015
Merged

Mal implementation by Crystal #70

merged 31 commits into from
Jun 3, 2015

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Jun 2, 2015

I implemented all steps of Mal by Crystal.

I checked that my implementation passed all test cases and self-hosted Mal implementation also passed in my OS X environment.

rhysd added 30 commits June 3, 2015 02:26
Crystal seems to treat 'Proc(Args..., Ret)' as '(Args...) -> Ret' but
distinguishes 'Proc(Args..., Ret)' from '(Args...) -> Ret'.
It causes a type-mismatch error.  I added some workarounds for it.
the test (def! res1 (sum-to 10000)) failed in my environment because it
could compute the result even if TCO is not available.  If stack
overflow happens, the interpreter will go into SEGV.
@kanaka
Copy link
Owner

kanaka commented Jun 3, 2015

@rhysd something seems to be off with printing:

$ crystal run ./step1_read_print.cr
user> 123
#<Mal::Type:0x2094f90>

$ crystal run ./stepA_mal.cr 
#<Mal::Type:0x17c4640>#<Mal::Type:0x17cf140>#<Mal::Type:0x17d4000>#<Mal::Type:0x17da240>#<Mal::Type:0x17dbc00>Mal [crystal]
user> 123
#<Mal::Type:0x17db0c0>#<Mal::Type:0x17e2fc0>

I tested with crystal-0.7.2 on Ubuntu 14.04.1

@rhysd
Copy link
Contributor Author

rhysd commented Jun 3, 2015

Sorry, previous deleted comments were my mistake. It seemed that I didn't notice the bug because of compiler's cache. I'll fix it.

@rhysd
Copy link
Contributor Author

rhysd commented Jun 3, 2015

It seems that print() is added in Crystal 0.7.2 and Mal's print is not called. I'll use Mal module to avoid that.

@rhysd
Copy link
Contributor Author

rhysd commented Jun 3, 2015

@kanaka

I fixed the bug then confirmed tests for step0 ~ stepA and self-hosted mal with Crystal 0.7.2.

kanaka added a commit that referenced this pull request Jun 3, 2015
Mal implementation by Crystal
@kanaka kanaka merged commit a06583f into kanaka:master Jun 3, 2015
@rhysd
Copy link
Contributor Author

rhysd commented Jun 3, 2015

Thank you!
I really enjoyed implementing mal.

@kanaka
Copy link
Owner

kanaka commented Jun 3, 2015

@rhysd You're welcome! Do you have any feedback on the guide? Especially places where you got stuck that the guide could have helped address?

I did a couple of minor cleanups to the Makefile. I also pushed an update to the README to bump the language count (35 now!) and to list you as the creator of this implementation.

The only thing left is for you to tweet about it and I'll retweet :-)

@rhysd
Copy link
Contributor Author

rhysd commented Jun 5, 2015

@kanaka

Sorry, I failed to catch your reply.

I did a couple of minor cleanups to the Makefile. I also pushed an update to the README to bump the language count (35 now!) and to list you as the creator of this implementation.

I see. Thank you for your work.

Do you have any feedback on the guide?

I think it is better to add more tests for object deepcopy.

Many languages (including Crystal) provide reference semantics, which employs shallow copy to copy array, dictionary and so on. It leads to shallow copy of mal's object and enables to change a value in environment from outside.
I met a bug that breaks environment in self-hosted mal and I worked a bit hard to fix it. Specifically, I forgot deepcopy for dictionary here. No test for previous steps couldn't find the shallow copy bug.

@kanaka
Copy link
Owner

kanaka commented Jun 5, 2015

@rhysd that's a good point. Do you have some example tests that would have caught the problem in your case?

micfan pushed a commit to micfan/make-a-lisp that referenced this pull request Nov 8, 2018
Mal implementation by Crystal
luelista pushed a commit to luelista/mal that referenced this pull request Mar 10, 2024
Mal implementation by Crystal
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