-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add support for Gray stream implementation of interactive-stream-p #640
Conversation
Also make the Java methods isInputStream, isOutputStream, isCharacterStream and isBinaryStream, sOpen call the appropriate Gray stream methods versus assuming specific inheritance.
= getCurrentPackage().findPackage("GRAY-STREAMS"); | ||
Symbol fundamentalInputStream | ||
= (Symbol) pkg.findSymbol("FUNDAMENTAL-INPUT-STREAM"); | ||
if (SUBTYPEP.execute(clos.typeOf(), fundamentalInputStream).equals(T)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yitzchak From what I remember, I think I had to add the SUBTYPEP to actually get this to work with uses of GRAY-STREAMS in some Quicklisp systems. I will try to go through my notes to figure out which systems I had problems with to see if your "terse" way works. In any event the use of getBooleanValue()
is definitely an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll see if I can add some tests to my suite looking for such things.
Also, do you want me to update the change log as I make PRs?
Sent from my iPad
On Nov 22, 2023, at 14:57, Tarn W. Burton ***@***.***> wrote:
Also, do you want me to update the change log as I make PRs?
That would be welcome, but do it in separate commits if possible please, otherwise merging commits can get needlessly complicated.
|
@yitzchak Finally found some time to test, and it seems the version of SLIME which uses GRAY-STREAMS won't run with this patch, as one gets the following stack trace.
One needs an unreleased version of SLIME past v2.28 that incorporates slime/slime@96de8b0. Previously ABCL had used a special, used-for-SLIME-only Java-side implementation of streams to communicate with SLIME (see https://github.com/armedbear/abcl/blob/master/src/org/armedbear/lisp/SlimeInputStream.java). With this patch, SLIME uses GRAY-STREAMS like most other implementations. Again, I think the problem with your patch stems from the removal of the SUBTYPEP checks. But now that I look at it, the problem might be able to be solved by changing the MAKE-TWO-WAY-STREAM implementation (cf. abcl/src/org/armedbear/lisp/TwoWayStream.java Line 227 in 6ae3f67
|
I think I see the better way to fix things.
Investigating a fix… |
Ok, thank you for that. On my side I have tested this on about a dozen systems that depend on trivial-gray-streams by loading and using those systems or by running the included tests. I haven't seen any issues yet. This isn't an exhaustive test but it included archive, babel, chunga, flexi-streams, and nibbles-streams. I also tested on the printer stack Incless/Inravina/Invistra which implements the CL printer and relies on Gray streams heavily. |
@yitzchak I've added patches on top of your work in https://github.com/easye/abcl/tree/gray-interactive to start to address the issue with use of SLIME, as well as being more robust in general. I am able to use SLIME with the ABCL GRAY-STREAM patch to connect, but the repl dies on the first interactive command issued I'm out of time at the moment to continue, so sharing where I've gotten. |
@easye I've reproduced your issues and I think I have a fix. I'll cherry pick your commits and update the PR later today. Also, I think slime needs some updates for ABCL. Specifically, I got an undefined symbol for |
Every Java-side function usually needs an explicit static autoload call.
@easye I've pushed a commit that fixed the issue for me. The commit history will need to be reworked and the change log updated. One thing I don't completely understand yet is why FIND-METHOD is needed at all. It seems like the current strategy will ignore specializations which are superclasses of the Gray stream instance and will also ignore specialization of the second argument for methods like STREAM-WRITE-SEQUENCE. Why can't we just call the generic functions directly? |
I don't know of a reason off hand, but I think I remember trying that early on in the fixes to GRAY-STREAMS for abcl-1.9.2, and it didn't work, so I ended up using FIND-METHOD. I would be in favor of using the generic methods directly, as your criticism that the current of use FIND-METHOD will not work for some specializations. Go ahead, and see if using the generic methods directly works for you? |
@easye Will do. |
This is the test matrix from my notes, which seems more-or-less the same as the systems you tested. I derived this list by looking at every Quicklisp system that references TRIVIAL-GRAY-STREAMS
|
SLIME is loading fine for me with your patch to my FIND-METHOD call missing the second argument 2f3cd57. Now I seem to be getting problems in using ASDF from the REPL, seeing errors like:
Interestingly, loading things via QL:QUICKLOAD still seems to work. |
Fixed this with easye@54f1023. Gonna do a little more testing, and push these changes. Afterwards, I will experiment with the direct use of generic functions, as "all" the error we seem to find here comes from the manual use of FIND-METHOD. |
Superseded by #641. Thanks again for the work. |
Also make the Java methods isInputStream, isOutputStream, isCharacterStream and isBinaryStream, isOpen call the appropriate Gray stream methods versus assuming specific inheritance.