Steel Bank Common Lisp

(CONCATENATE 'NULL ...) does not handle generic sequences

Reported by Jan Moringen on 2013-03-30
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SBCL
Undecided
Unassigned

Bug Description

What I do:
(defclass list-backed-sequence (standard-object
                                sequence)
  ((elements :initarg :elements :type list :accessor %elements)))

(defmethod sequence:make-sequence-like ((sequence list-backed-sequence) length &rest args &key initial-element initial-contents)
  (declare (ignore initial-element initial-contents))
  (make-instance 'list-backed-sequence
                 :elements (apply #'sequence:make-sequence-like '() length args)))

(defmethod sequence:length ((sequence list-backed-sequence))
  (length (%elements sequence)))

(defmethod sequence:elt
    ((sequence list-backed-sequence) index)
  (nth index (%elements sequence)))

(defmethod (setf sequence:elt)
    (new-value (sequence list-backed-sequence) index)
  (setf (nth index (%elements sequence)) new-value))

(concatenate 'null (coerce '() 'list-backed-sequence))

What happens:
debugger invoked on a SIMPLE-TYPE-ERROR in thread
#<THREAD "main thread" RUNNING {AB3BDB9}>:
  The length requested (0) does not match the type restriction in NULL.

What did I expect to happen:
CONCATENATE should return nil.

SBCL version: ~ 1.1.6

Jan Moringen (scymtym) wrote :

The attached patch introduces a new sequence function EMPTYP which (CONCATENATE 'NULL ...) uses.

Jan Moringen (scymtym) on 2013-04-18
tags: added: review
Paul Khuong (pvk) wrote :

I'd personally prefer if SB-SEQUENCE:EMPTYP returned a boolean rather than a generalised boolean, but that's really not major.

We usually refer the launchpad ticket directly in test cases.

Otherwise, it looks good, except that I have no clue on format trickery (:

Changed in sbcl:
assignee: nobody → Paul Khuong (pvk)
Jan Moringen (scymtym) wrote :

Thanks for the review. Updated patch is attached.

> I'd personally prefer if SB-SEQUENCE:EMPTYP returned a boolean rather than a generalised boolean, but that's really not major.

Done.

> We usually refer the launchpad ticket directly in test cases.

I changed the patch to reference the ticket in NEWS and WITH-TEST.

I did not reference the bug in the test case because it tests multiple things. Now the test case has the function, tested aspect and the bug in its name.

> Otherwise, it looks good, except that I have no clue on format trickery (:

I left the changed format string and review question in the patch, but maybe it can be committed without that part.

Jan Moringen (scymtym) wrote :

One final revision: avoid test name seq-test::emptyp to not confuse test runner.

That looks good. I'll commit it in the next release.

Paul Khuong (pvk) wrote :

Also, ZEROP returns a boolean on SBCL, so the default implementation can be slightly simpler.

Paul Khuong (pvk) on 2013-05-19
Changed in sbcl:
status: New → In Progress
Paul Khuong (pvk) wrote :

Committed in 22c592c.

Changed in sbcl:
status: In Progress → Fix Committed
assignee: Paul Khuong (pvk) → nobody
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers