ClojureScript

The pop function in PersistentVector is buggy

Details

  • Type: Defect Defect
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:

Description

Example:

(conj (pop [1 2]) 3) => [1 2]

Attachments

  1. File
    fix-persistentvector-pop.diff
    19/Apr/12 3:48 AM
    1.0 kB
    Nicola Mometto
  2. File
    fix-persistentvector-pop.diff
    18/Apr/12 12:50 PM
    1 kB
    Nicola Mometto
  3. File
    fix-persistentvector-pop-with-tests.diff
    19/Apr/12 10:15 AM
    2 kB
    Nicola Mometto

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Nicola Mometto added a comment -

this patch fixes pop

Show
Nicola Mometto added a comment - this patch fixes pop
Nicola Mometto made changes -
Field Original Value New Value
Attachment fix-persistentvector-pop.diff [ 11061 ]
Nicola Mometto made changes -
Attachment fix-persistentvector-pop.diff [ 11061 ]
Nicola Mometto made changes -
Attachment fix-persistentvector-pop.diff [ 11062 ]
Hide
Permalink
Laszlo Török added a comment -

Hi,

good catch, but why not simply:

(PersistentVector. meta (dec cnt) shift root (.slice tail 0 -1))
;; you don't need to aclone and splice, .slice returns a new array

Show
Laszlo Török added a comment - Hi, good catch, but why not simply: (PersistentVector. meta (dec cnt) shift root (.slice tail 0 -1)) ;; you don't need to aclone and splice, .slice returns a new array
Hide
Permalink
Nicola Mometto added a comment -

right, using slice is even faster.
patch updated

Show
Nicola Mometto added a comment - right, using slice is even faster. patch updated
Nicola Mometto made changes -
Attachment fix-persistentvector-pop.diff [ 11066 ]
Hide
Permalink
Laszlo Török added a comment -

Nice, thank you!

Could you please add a test that covers the issue?

e.g.

(assert (= (vec (range 33))
(-> (vec (range 33))
(pop)
(pop)
(conj 31)
(conj 32))))

Show
Laszlo Török added a comment - Nice, thank you! Could you please add a test that covers the issue? e.g. (assert (= (vec (range 33)) (-> (vec (range 33)) (pop) (pop) (conj 31) (conj 32))))
Hide
Permalink
Nicola Mometto added a comment -

sure, here it is

Show
Nicola Mometto added a comment - sure, here it is
Nicola Mometto made changes -
Attachment fix-persistentvector-pop-with-tests.diff [ 11067 ]
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.