search

joaotavora / sly

Sylvester the Cat's Common Lisp IDE
1.24k stars 140 forks source link

Fix the ABCL Backend #485

Closed gwbrown closed 1 year ago

gwbrown commented 2 years ago

This PR fixes the issues with the ABCL backend described in https://github.com/joaotavora/sly/issues/425, as well as several other bugs and issues such as https://github.com/slime/slime/issues/649, and removes a nonfunctional feature or two.

Unlike https://github.com/joaotavora/sly/pull/449, this PR does not introduce any new files; though the load order is adjusted slightly to allow the backend to make use of slynk-match, the rest of the load order is untouched and the other uses of functions that are not defined at the time the backend is loaded are dealt with using a hack to read the symbol at runtime. This hack is as constrained as possible so it shouldn't spread too far and should be easy to replace.

I've tried to break this out into relatively sensical commits with more details about each change.

joaotavora commented 2 years ago

This looks promising, Gordon. I'll look at this asap and get back to you. Thanks for this work!

joaotavora commented 2 years ago

I've had a look and it looks very nice. If you want to address the very minor nits I commented on, feel free to. Else, I would just ask you to prefix all the 5/6 commits with Fix #485: .... and a GNU ChangeLog style line to the body of the commit message like so that it looks like:

Fix #485: Do thingy to ABCL

* slynk/backend/abcl.lisp (some-relevant-function-or-definition): Brief description of what was done here.

I don't want to hold up this merge just because of these details, so let me know if you'd like me to this these administrative things myself (in which case I will rewrite your commits while keeping your authorship, of course).

gwbrown commented 2 years ago

Thanks for the review, I'll address your comments soon, probably today including the commit messages.

mdbergmann commented 2 years ago

Been a while. What's the state @gwbrown ?

joaotavora commented 1 year ago

I finally got around to review this. It is well crafted and documented. I enhanced the commit messages with some more information and am merging this. My only worry is that the change to slynk/slynk.asd breaks some other backends, but after some minor testing it doesn't seem to. So thanks very much, @gwbrown.