Improve the clasp slynk backend and fix a non-standard loop macro #676
Conversation
The search-for-recording-1 function depended on nonconforming behavior of the LOOP macro.
Upgraded the clasp.lisp backend to work with sly.
Clasp will set *compile-file-truname* to /tmp/xxxx and that won't contain 'sly.el' but *load-truename* will contain the path to slynk-loader.lisp
It's better to print ',name than ',sym
| ;; this funky scheme has something to do with rollover | ||
| ;; semantics probably | ||
| ;; | ||
| for inc in `(,increment ,(if (plusp increment) 1 -1)) |
There was a problem hiding this comment.
Here's the thing: I haven't done much Common Lisp recently, so it's hard for me to review this change.
But SLY (from master) is still heavily used around some circles as far as I know. So I'd like to know what % you are sure this is a functionally equivalent (and fully conformant) loop invocation. It seems to work fine in all implementations so far except for Clasp. How deeply have you tested this?
There was a problem hiding this comment.
I tested the code I wrote with sbcl, ecl, and clasp and it works fine.
The original loop code was not conformant. To summarize, FOR clauses cannot appear after WHILE clauses. FOR clauses are variable-clauses and WHILE clauses are termination-tests, which are main-clauses, which follow variable-clauses.
Check out:
https://www.lispworks.com/documentation/HyperSpec/Body/m_loop.htm
It says:
The "extended" [loop](https://www.lispworks.com/documentation/HyperSpec/Body/m_loop.htm#loop) [form](https://www.lispworks.com/documentation/HyperSpec/Body/26_glo_f.htm#form):
loop [name-clause] {variable-clause}* {main-clause}* => result*
...
variable-clause::= with-clause | initial-final | for-as-clause
...
main-clause::= unconditional | accumulation | conditional | termination-test | initial-final
...
termination-test::= while form | until form | repeat form | always form | never form | thereis form
| (require :profile) | ||
| (pushnew :profile *features*)) | ||
| (when (probe-file "sys:serve-event") | ||
| (when (probe-file "sys:src;lisp;modules;serve-event;") |
There was a problem hiding this comment.
Thanks very much for all your work on this file. I won't review it, but I trust it's the right thing every time :-)
There was a problem hiding this comment.
Ok, thanks. Yeah, I'll deal with issues in clasp.lisp
| "Return a string identifying the SLY version. | ||
| Return nil if nothing appropriate is available." | ||
| (let ((this-file #.(or *compile-file-truename* *load-truename*))) | ||
| (let ((this-file #.(or #-clasp *compile-file-truename* *load-truename*))) |
There was a problem hiding this comment.
OK. I'm reading the explanation in the commit message and I think I get it. But that message should be even clearer. if this rewording is correct, use something like it instead :
Clasp *compile-file-truename* will be /tmp
Clasp will set *compile-file-truename* to /tmp/something.faslclasp
the containing directory of which likely won't contain 'sly.el'. However
*load-truename* will contain the path to slynk-loader.lisp, and 'sly.el'
should be found as a result.
Also, as far as I remember loading slynk-loader.lisp is just another "legacy" way of loading SLY. The preferred way is to load it via ASDF. Does this work with Clasp? Did you test it?
There was a problem hiding this comment.
Ok, how would I change the commit message? Or can you change it. Your version is more readable.
| (setq *unimplemented-interfaces* | ||
| (remove ',sym *unimplemented-interfaces*)) | ||
| (warn "DEFIMPLEMENTATION of undefined interface (~S)" ',sym)) | ||
| (warn "DEFIMPLEMENTATION of undefined interface (~S)" ',name)) |
There was a problem hiding this comment.
I think this makes sense. Can you give a very summarized before&after of what this change does to the logs?
There was a problem hiding this comment.
The DEFIMPLEMENTATIONs that I removed from clasp.lisp triggered this warning and they read:
WARNING: DEFIMPLEMENTATION of undefined interface (NIL)
, which wasn't useful. After this change you will see warnings like:
WARNING: DEFIMPLEMENTATION of undefined interface (DUMMY-IMPLEMENTATION)
I tested this on sbcl and in clasp.
| "Return two values: a RECORDING and its position in *RECORDINGS*. | ||
| Start searching from position FROM, an index in *RECORDINGS* which is | ||
| successfuly increased by INCREMENT before using that to index | ||
| successively increased by INCREMENT before using that to index |
There was a problem hiding this comment.
Thanks, good catch. I think you can squash this commit in the other one where you fix the loop to be conforming. If you're not comfortable enough with Git to do that, I can easily do it for you.
There was a problem hiding this comment.
I'm not comfortable enough with git to do this - can you?
Git and I have an uneasy relationship.
|
I think I responded to all your questions. |
|
I completely forgot about this... What a shame. Pinging myself so I don't forget. |
2 participants
I've upgraded the clasp slynk backend so that clasp can make full use of sly's enhancements.
I also found a loop macro in search-for-recording-1 that put a while clause before some for clauses and that gives our symbolics loop macro some trouble.