Hi all,
I inadvertently discovered some bugs in the gray-streams implementation due to a stale flexi-streams that did not get upgraded properly.
There were quite a few things that needed tidying - the type checking using EQ is wrong. Also, after trying to hunt around CLHS, I think BYTE is not a CL type (though I may be mistaken). This could be a reflection of some Corman Lisp specific feature in the past (Yes! These bugs seem to have been around ABCL for that long!)
Some test examples (avoiding flexi-streams or other dependencies)
;; ----- setup
(require 'gray-streams)
(defclass test-gray-binary-input-stream (gray-streams:fundamental-binary-input-stream) ())
(defclass test-gray-character-input-stream (gray-streams:fundamental-character-input-stream) ())
(let ((bytes (list 65 66 67 68 69)) (pos -1)) (defmethod gray-streams::stream-read-byte ((stream test-gray-binary-input-stream)) (elt bytes (mod (incf pos) 5))))
(let ((chars (list #\A #\B #\C #\D #\E)) (pos -1)) (defmethod gray-streams::stream-read-char ((stream test-gray-character-input-stream)) (elt chars (mod (incf pos) 5))))
;; -----
(let ((s (make-instance 'test-gray-binary-input-stream))) (loop repeat 10 collect (read-byte s))) => (65 66 67 68 69 65 66 67 68 69)
(let ((arr (make-array 10 :initial-element nil))) (list (read-sequence arr (make-instance 'test-gray-binary-input-stream) :start 2 :end 7) arr))
Expected (7 #(NIL NIL 65 66 67 68 69 NIL NIL NIL)), but got an error instead, ... no applicable method...
I guess implementors of gray streams would normally define both read-byte & read-sequence. But ABCL's default/fallback method does try to support binary streams, it just isn't being dispatched on the right class.
Also, read-sequence should return the "final" array index, not the number of bytes/characters read. Similarly, write-sequence needs to do its work, then return the sequence itself.
Attached is a patch with what I hope are the required fixes, please review.
Two additional notes,
1. Even after loading my proposed fixes, I'd would still get this, which is ok, but the error message is not obvious enough. This is actually a method not found error, not a type error. There appears to be some type guessing going on, so perhaps the message should indicate failure to dispatch on STREAM or similar.
(gray-streams::stream-element-type (make-instance 'test-gray-binary-input-stream)) The value #<TEST-GRAY-BINARY-INPUT-STREAM {500BBBF9}> is not of type STREAM. [Condition of type TYPE-ERROR]
So we just need something like this,
(defmethod gray-streams::stream-element-type ((stream test-gray-binary-input-stream)) '(unsigned-byte 8))
2. I also don't understand this snippet in the original code, converting unsigned-bytes/integers (basically the wrong type) to a character?
(#+nil ccl:int-char code-char (elt sequence n))
Of course, after patching the above, I discovered that re-compiling flexi-streams got rid of my particular set of problems :-)
Yong
On Jan 16, 2014, at 13:44, Theam Yong Chew senatorzergling@gmail.com wrote:
Hi all,
I inadvertently discovered some bugs in the gray-streams implementation due to a stale flexi-streams that did not get upgraded properly.
Applied in [r14602][1]: FLEXI-STREAMS from Quicklisp passes all its tests! Fantastic!
[1]: http://abcl.org/trac/changeset/14602
16.01.2014, 21:22, "Mark Evenson" evenson@panix.com:
On Jan 16, 2014, at 13:44, Theam Yong Chew senatorzergling@gmail.com wrote:
Hi all,
I inadvertently discovered some bugs in the gray-streams implementation due to a stale flexi-streams that did not get upgraded properly.
Applied in [r14602][1]: FLEXI-STREAMS from Quicklisp passes all its tests! Fantastic!
Flexi-streams test suite passes even on ABCL 1.1.1 http://common-lisp.net/project/cl-test-grid/library/flexi-streams.html
But not in quicklisp 2014-01-13, where flexi-streams test suite has new test case. It fails on ABCL due to #342: http://cl-test-grid.appspot.com/blob?key=1r0ys54trc
So, I suggest to commit the trivial fix from #342.
Note about the Yong's patch. Strictly speaking, there is not requirement that gray streams implementation provide default method stream-read-sequence implemented in terms of read-char (for character streams) or read-byte (for binary streams).
Gray proposal does not contain stream-read-sequence function at all: http://www.nhplace.com/kent/CL/Issues/stream-definition-by-user.html
I suppose it is because CLTL book doesn't have cl:read-sequence: http://www.cs.cmu.edu/Groups/AI/html/cltl/clm/node194.html#SECTION0026200000...
But of course, it reasonable to have default methods.
Best regards, - Anton
On Jan 17, 2014, at 1:05, Anton Vodonosov avodonosov@yandex.ru wrote:
16.01.2014, 21:22, "Mark Evenson" evenson@panix.com:
On Jan 16, 2014, at 13:44, Theam Yong Chew senatorzergling@gmail.com wrote:
Hi all,
I inadvertently discovered some bugs in the gray-streams implementation due to a stale flexi-streams that did not get upgraded properly.
Applied in [r14602][1]: FLEXI-STREAMS from Quicklisp passes all its tests! Fantastic!
Flexi-streams test suite passes even on ABCL 1.1.1 http://common-lisp.net/project/cl-test-grid/library/flexi-streams.html
But not in quicklisp 2014-01-13, where flexi-streams test suite has new test case. It fails on ABCL due to #342: http://cl-test-grid.appspot.com/blob?key=1r0ys54trc
So, I suggest to commit the trivial fix from #342.
For some reason, applying your patch in [#342][1] seems to cause the ansi-compiled tests to fail to execute.
[1]: http://abcl.org/trac/ticket/342
On Jan 17, 2014, at 13:51, Mark Evenson evenson@panix.com wrote:
On Jan 17, 2014, at 1:05, Anton Vodonosov avodonosov@yandex.ru wrote:
Flexi-streams test suite passes even on ABCL 1.1.1 http://common-lisp.net/project/cl-test-grid/library/flexi-streams.html
But not in quicklisp 2014-01-13, where flexi-streams test suite has new test case. It fails on ABCL due to #342: http://cl-test-grid.appspot.com/blob?key=1r0ys54trc
So, I suggest to commit the trivial fix from #342.
For some reason, applying your patch in [#342][1] seems to cause the ansi-compiled tests to fail to execute.
After re-establishing my test environment (and testing on another machine), whatever was causing my error in the ansi-compiled tests has disappeared, so I have committed as [r14605][].
Thanks for the patch (and perseverance)!
[r14605]: http://abcl.org/trac/changeset/14605
On 1/17/14, Anton Vodonosov avodonosov@yandex.ru wrote:
16.01.2014, 21:22, "Mark Evenson" evenson@panix.com:
On Jan 16, 2014, at 13:44, Theam Yong Chew senatorzergling@gmail.com wrote:
Hi all,
I inadvertently discovered some bugs in the gray-streams implementation due to a stale flexi-streams that did not get upgraded properly.
Applied in [r14602][1]: FLEXI-STREAMS from Quicklisp passes all its tests! Fantastic!
Flexi-streams test suite passes even on ABCL 1.1.1 http://common-lisp.net/project/cl-test-grid/library/flexi-streams.html
That's right. Just a quick note, I was going to say I didn't deserve any credit. What really happened to me was the result of an upgrade from a very old quicklisp. trivial-gray-streams was upgraded, but somehow my flexi-streams didn't recompile itself. The stale .abcls had a different superclass hierarchy, mucking up the method dispatch. This resulted in ABCL's own gray-streams methods being invoked.
Since in practice, most folks would implement both read-char & read-sequence together (including flexi-streams), the ABCL gray streams probably never got used thus far. That's why the bugs were never uncovered. No bugs (not related to my topic anyway :-)) were uncovered in flexi-streams or ABCL's support of flexi-streams this time.
Yong
17.01.2014, 15:18, "Theam Yong Chew" senatorzergling@gmail.com:
That's right. Just a quick note, I was going to say I didn't deserve any credit.
Yong, I am sorry, I didn't mean you don't deserve credit, the default stream-read-sequence you implemented is an improvement for ABCL.
Best regards, - Anton
armedbear-devel@common-lisp.net