-
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
Implement suggestions from issue #587 in stepA of several languages #592
base: master
Are you sure you want to change the base?
Conversation
4abbbe9
to
3dad5f7
Compare
It's been a few months since I've been able to do anything mal related. After mulling this change for a while, I think I've decided I now like the idea. It does simplify the flow quite a bit and in particular, I think it aligns well with mal's first class macro functions. And I agree with I think the main change I would like to request is that we keep some form of user invokable macro expansion. I think this is an important tool for learning/pedagogy (in addition to debugging). I do agree that it should only expand one level (so renaming it to "macroexpand-1" is probably good). If you can think of a clean/nice way to use your new mechanism/flow then that would be great, otherwise, I'm fine with keeping the separate If you're still willing to push this forward (applying the changes to step8 and 9 and fixing conflicts caused earlier merges), then I think this is nearly ready for merging. We can create a new low-priority ticket with a list of implementations remaining to convert to the new model (if ever). |
3dad5f7
to
aca60f6
Compare
Please also comment on the changes in The macro changes affect steps 8-A, but the eval-ast changes affect steps 2-A. About In my opinion, it is better that each EVAL outputs its AST, not only macro expansions. The log is noisy, but consistent with the experience in previous steps for new implementors. A different question is "how should this echo be enabled"? It is convenient to enable the echo interactively without restarting the interpreter. This is what
A good compromise would allow one to enable the echo without editing the source code. Since command line arguments are already mandatory, we could for example let step6 replace the Most languages already embed a print command in comments, but this is not convenient, especially with compiled languages. Also, it has already happened that the command is obsolete when one actually requires it. |
aca60f6
to
e612101
Compare
I reviewed the One thing I realized this weekend is that the diagrams will need to be updated because they refer to eval-ast. I might take the opportunity myself to port the diagrams from gliffy to drawio so they are easier to update with this and future changes. Configurable debug is a great idea. I had an idea for how to do it: diff --git a/impls/python/step3_env.py b/impls/python/step3_env.py
index 75ec834e..2e0bfe4d 100644
--- a/impls/python/step3_env.py
+++ b/impls/python/step3_env.py
@@ -10,7 +10,9 @@ def READ(str):
# eval
def EVAL(ast, env):
- #print("EVAL %s" % printer._pr_str(ast))
+ edbg = env.get(types._symbol('*DEBUG-EVAL*'))
+ if edbg != None and edbg != False:
+ print("EVAL %s" % printer._pr_str(ast))
if types._symbol_Q(ast):
return env.get(ast)
@@ -50,6 +52,7 @@ repl_env = Env()
def REP(str):
return PRINT(EVAL(READ(str), repl_env))
+repl_env.set(types._symbol('*DEBUG-EVAL*'), False)
repl_env.set(types._symbol('+'), lambda a,b: a+b)
repl_env.set(types._symbol('-'), lambda a,b: a-b)
repl_env.set(types._symbol('*'), lambda a,b: a*b) This enables per lexical scope debug but also global debug if desired:
Obviously the same idea would be propagated forwarded from step3 and described in the process as an optional but useful thing to implement at step3 (with an optional test case to make sure it's doing the right thing). And step6 could support a command line parameter for enabling the global setting by default (again, optional). One thing I really like about it is that it provides a template for more narrower debug (i.e. implementors could create other While I like the dynamically enabled debug flag idea and think it's definitely worthwhile, I'm not completely convinced though that this fully replaces |
e612101
to
9d25f42
Compare
Hello. |
e8245a8
to
a1692c1
Compare
Hello. c.2: the failure on github is unreproducible with gcc 11.12.0. elm: the failure is most probably unrelated because the directory is unchanged. |
241500c
to
24a0c01
Compare
ce57925
to
750e8d8
Compare
Some tests were failing with Make 4.3.
See issue kanaka#587. * Merge eval-ast and eval into a single conditional. * Expand macros during the apply phase, removing lots of duplicate tests, and increasing the overall consistency by allowing the macro to be computed instead of referenced by name (`((defmacro! cond (...)))` is currently illegal for example). * Print "EVAL: $ast" at the top of EVAL if DEBUG-EVAL exists in the MAL environment. * Remove macroexpand and quasiquoteexpand special forms. * Use pattern-matching style in process/step*.txt. Unresolved issues: c.2: unable to reproduce with gcc 11.12.0. elm: the directory is unchanged. groovy: sometimes fail, but not on each rebuild. nasm: fails some new soft tests, but the issue is unreproducible when running the interpreter manually. objpascal: unreproducible with fpc 3.2.2. ocaml: unreproducible with 4.11.1. perl6: unreproducible with rakudo 2021.09. Unrelated changes: Reduce diff betweens steps. Prevent defmacro! from mutating functions: c forth logo miniMAL vb. dart: fix recent errors and warnings ocaml: remove metadata from symbols. Improve the logo implementation. Encapsulate all representation in types.lg and env.lg, unwrap numbers. Replace some manual iterations with logo control structures. Reduce the diff between steps. Use native iteration in env_get and env_map Rewrite the reader with less temporary strings. Reduce the number of temporary lists (for example, reverse iteration with butlast requires O(n^2) allocations). It seems possible to remove a few exceptions: GC settings (Dockerfile), NO_SELF_HOSTING (IMPLS.yml) and step5_EXCLUDES (Makefile.impls) .
750e8d8
to
225d450
Compare
This is required by the current mal implementation of DBG-EVAL.
See kanaka#592 for context.
Hello. |
Related with issue #587 .
Merge eval-ast and eval into a single conditional.
Print "EVAL: $ast" at the top of EVAL.
Expand macros during the apply phase, removing lots of duplicate
tests, and increasing the overall consistency by allowing the macro
to be computed instead of referenced by name (
((defmacro! cond (...)))
is currently illegal for example).Each change was already applied by some implementations.
With the two last changes, macroexpand and quasiquoteexpand special
forms are not needed anymore.
Not tested locally: common-lisp, julia, io, logo, lua, matlab, vhdl,
vimscript
If macroexpand is kept:
an optional test should be added for
(macroexpand ())
, whichcurrently fails on several implementations,
it should only apply one macro at a time. The purpose being
debugging, hiding an iteration does not help. Moreover, the loop is
currently untested (and probably wrong in nim).