-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improve mal impl macro no meta #400
Improve mal impl macro no meta #400
Conversation
@asarhaddon okay, I'm finally get back around to reviewing some of the mal/macro/quasiquote related changes. I'd like to start with getting this one in first. I'm fine with the approach and I think removing the need for metadata is a big plus. There are two minor things that I would like changed:
I think probably the simplest solution is to just retain the previous behavior by adding a _macro_unwrap check to PRINT (for steps 8-A) and printing that if it's "true" otherwise printing the whole thing.
There are also some other behavior changes (such as being able to use macros with sequence functions first, rest, nth, etc) but I'm not really concerned about those and don't think it's worth adding any additional complexity to make them explicitly check and throw errors. |
Oh, I just realized this is a pull request on the "self-host-test" branch. I should really delete that branch because it's just confusing things. It was an experiment that kind of dead-ended. Instead, I have a script in tests/travis_trigger.sh that I can use to re-trigger a Travis build with custom configuration. Unfortunately, Travis is pretty limited and I think I'm the only one that can use it against my branches. :-( Looks like #401 is also targeted at self-host-test. Anyways, can you re-target this at master (and merge if needed since master has since moved on)? Once you have done that I'll trigger a custom build in self-host mode. If that passes, then I'll merge. Sorry about the confusion. |
Generally: remove variables used only once with generic names, introduce a variable when the same result is computed twice. core_rs: replace strings with symbols because it is more consistent with the public interface of `env.mal` bind-env: - compare with '& instead of "&", avoiding a conversion per iteration - compute `first` once, store the result env-find: - move `if env` at start of recursion, avoiding to `get` in a row. stepA_mal: - eval-ast, eval: remove duplicate `do` - LET: move `form` into the signature. The recursion does not change much, but the initial call is shorter and more intuitive. - EVAL: - remove initial (not (list? ast)) test, which was redundant (MACROEXPAND would do nothing). - replace (nil? (first ast)) with (empty? ast), more explicit (also, `(nil 1)` is now reported as incorrect - `try*`: stop checking that first component of optional argument is `catch*`. No other user input is checked explicitly. - repl-loop: a slight modification avoids to create a new environment depth for each new line.
* Move `gensym` and `inc` from step files to `lib/trivial.mal`. * Move `or` from step files to `lib/test_cascade.mal`. Shorten it because `(first ())` returns `nil` * Update process and tests accordingly (not the figures yet).
Make: avoid # character. Guile: avoid `unquote` inside a vector inside a list inside `quasiquote`. The bug in scheme/ is most probably the same.
Unlike the one in `env.mal`, the `get` built-in used during step2 returns `nil`, so the MAL implementation must throw an error.
Let `with-meta f m` create a function even if f is a macro, instead of setting an "ismacro" metadata that is never used again (and breaks self-hosting). Also move the test for fn? from Optional to Deferrable, the function is used for self-hosting.
Support for metadata becomes optional. Support for fn? becomes optional again, reverting 5e5d489.
0b3f3d4
to
cb9b065
Compare
Self-hosted test is running here: https://travis-ci.org/kanaka/mal/builds/556847476 Looking good so far. |
So three implementations failed self-host. I did a self-host build of just those three but without these changes and they passed (https://travis-ci.org/kanaka/mal/builds/556920394), so something about these changes is breaking those three. Also my earlier suggestion for printing is incomplete because it doesn't catch functions embedded in other things. i.e. All that to say, I think I'm reconsidering the special printing and leaning towards just printing the underlying object directly. However, I think I would like to switch to using a map rather than a vector as the container so it appears a bit more object-like. Something like this: {:__MAL_MACRO__ <function>}
|
Output of macros will probably be more readable. Inline _macro_wrap and _unwrap for efficiency (there are less primitive operations for maps than for vectors). Basic check of `map?` and `macro?`. Swap them in order to simplify the diff with cb9b065.
Adapting |
Changes look good. The regular tests pass. Running self-host tests again: https://travis-ci.org/kanaka/mal/builds/558757626 |
@asarhaddon There are 7 languages that fail now. I pushed the fix you suggested for powershell and that seems to have addressed that one. I poked at the haxe failure for a few minutes and discovered that this change fixes the the rest of the failures apart from rexx and nasm:
I think that indicates that there is probably still some hidden bugs in those implementations around comparison (or the definition of not). However, I won't have time any time soon to track those down and the the change is more efficient that doing = on a collection. Interestingly, the nasm failure appears to be because the step9_try.mal file is exactly 4096 bytes. Adding or removing a character anywhere in the file fixes the issue (likewise, padding step1 to 4096 makes the problem happen there too). @bendudson can you take a look at why nasm crashes when trying to do load-file of a file that is exactly 4096 bytes?
|
This is more efficient, and avoids triggering some bugs (see kanaka#400).
…-impl-macro-no-meta
The issue with vala seems different. An assertion fails during the initial traversal of |
Good idea. Done: #418 |
New self-hosted Travis build running: https://travis-ci.org/kanaka/mal/builds/559063412 |
Okay, since the only two failures now are nasm and vala and those appear to be issues discovered by the changes rather than introduced by the changes (and we are tracking those separately in #418), I'm going to do one more quick review of the code and then merge if nothing big sticks out at me. |
Merged. It's a big diff, but the majority of it is simplifying individual implementations by removing inc, gensym, and or definitions. And I think removing the need for metadata from the self-host requirements is a good improvement that remove incidental complexity from necessary steps the process/guide without losing any of the value. Thanks for persevering and pushing this through! |
This pull request should trigger tests for the changes discussed at #384.