Clojure

Collection.toArray implementations do not conform to Java API docs

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Per http://docs.oracle.com/javase/6/docs/api/index.html?java/util/Collections.html, Collection.toArray(T []) implements must allocate an array of type T[], not Object[]. This will cause type-casty code to break. Not often a problem in Clojure, but e.g. it makes Clojure defrecords unprintable from the JRuby console.

There is also a chance to DRY up some common code, as the six places that have this problem should all call an RT helper.

This also fixes toArray(T[]) for PersistentQueue when the passed array has the same length as the queue.

Attachments

  1. Text File
    0001-CLJ-895.-Use-RT.seqToPassedArray-to-implement-Collec.patch
    03/Feb/12 6:52 PM
    6 kB
    Cosmin Stejerean
  2. Text File
    0895-take-4.patch
    19/Feb/12 6:48 PM
    6 kB
    Stuart Halloway
  3. Text File
    0895-take-5.patch
    19/Feb/12 7:17 PM
    6 kB
    Stuart Halloway
  4. Text File
    0895-to-array.patch
    10/Dec/11 5:35 PM
    7 kB
    Stuart Halloway
  5. Text File
    0895-to-array-take-2.patch
    11/Dec/11 6:53 AM
    7 kB
    Stuart Halloway

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Stuart Halloway added a comment -

Sorry about the ^M crap in the patch, feel free to humiliate me on twitter with the correct incantation to prettify patch whitespace.

Show
Stuart Halloway added a comment - Sorry about the ^M crap in the patch, feel free to humiliate me on twitter with the correct incantation to prettify patch whitespace.
Hide
Permalink
Cosmin Stejerean added a comment - - edited

I don't understand the motivation for changing toArray in PersistentList. Calling seqToPassedArray here seems to accomplish the same thing, but the way in which it does that becomes harder to follow (at least for me). I would be tempted to keep the old toArray code in PersistentList.

                                                                                                                                                                                              
-       public Object[] toArray(Object[] objects){                                                                                                                                                          
-               if(objects.length > 0)                                                                                                                                                                      
-                       objects[0] = null;                                                                                                                                                                  
-               return objects;                                                                                                                                                                             
+       public Object[] toArray(Object[] a){                                                                                                                                                                
+        return RT.seqToPassedArray(seq(), a);                                                                                                                                                              
        }

Also, in the implementation of seqToPassedArray I think the final if condition can be simplified because dest != passed only if len > passed.length. I think this would do.

+        if (len < passed.length) {
+            dest[len] = null;
+        }

Finally, it probably wouldn't hurt to throw some braces around the for loop.

Show
Cosmin Stejerean added a comment - - edited I don't understand the motivation for changing toArray in PersistentList. Calling seqToPassedArray here seems to accomplish the same thing, but the way in which it does that becomes harder to follow (at least for me). I would be tempted to keep the old toArray code in PersistentList. 
                                                                                                                                                                                              
-       public Object[] toArray(Object[] objects){                                                                                                                                                          
-               if(objects.length > 0)                                                                                                                                                                      
-                       objects[0] = null;                                                                                                                                                                  
-               return objects;                                                                                                                                                                             
+       public Object[] toArray(Object[] a){                                                                                                                                                                
+        return RT.seqToPassedArray(seq(), a);                                                                                                                                                              
        }
Also, in the implementation of seqToPassedArray I think the final if condition can be simplified because dest != passed only if len > passed.length. I think this would do. 
+        if (len < passed.length) {
+            dest[len] = null;
+        }
Finally, it probably wouldn't hurt to throw some braces around the for loop.
Hide
Permalink
Stuart Halloway added a comment -

Thanks Cosmin. "take-2" patch incorporates your suggestions.

Show
Stuart Halloway added a comment - Thanks Cosmin. "take-2" patch incorporates your suggestions.
Hide
Permalink
Cosmin Stejerean added a comment -

I removed the line ending changes from PersistentQueue from the patch so now just the relevant changes should be left.

Show
Cosmin Stejerean added a comment - I removed the line ending changes from PersistentQueue from the patch so now just the relevant changes should be left.
Hide
Permalink
Cosmin Stejerean added a comment - - edited

Added tests for the new toArray functionality. Checking for collections shorter, same-length, and longer than the passed array for lists, vectors, hash-sets and queues.

In the process discovered that the previous implementation of toArray for PersistentQueue was actually broken when the passed array had the same length as the queue because

if(a.length >= count())
    a[count()] = null;

should have been

if(a.length > count())
    a[count()] = null;

which was the case for the other implementations. With the new shared implementation of RT.seqToPassedArray this is no longer a problem.

Show
Cosmin Stejerean added a comment - - edited Added tests for the new toArray functionality. Checking for collections shorter, same-length, and longer than the passed array for lists, vectors, hash-sets and queues. In the process discovered that the previous implementation of toArray for PersistentQueue was actually broken when the passed array had the same length as the queue because 
if(a.length >= count())
    a[count()] = null;
should have been 
if(a.length > count())
    a[count()] = null;
which was the case for the other implementations. With the new shared implementation of RT.seqToPassedArray this is no longer a problem.
Hide
Permalink
Stuart Sierra added a comment -

Patch 0895-to-array-take-3.patch does not apply on 485b6fc357157458f156bbba49f8e78c10c66c31

Show
Stuart Sierra added a comment - Patch 0895-to-array-take-3.patch does not apply on 485b6fc357157458f156bbba49f8e78c10c66c31
Hide
Permalink
Cosmin Stejerean added a comment -

Odd, it seems to apply cleanly for me onto 485b6fc357157458f156bbba49f8e78c10c66c31 as well as 12f07da889819bc5613546ec223e97ac27c86dbf (current HEAD).

$ git reset --hard HEAD
HEAD is now at 485b6fc [maven-release-plugin] prepare for next development iteration
$ git checkout 485b6fc357157458f156bbba49f8e78c10c66c31
HEAD is now at 485b6fc... [maven-release-plugin] prepare for next development iteration
$ patch -p1 < 0895-to-array-take-3.patch 
patching file src/jvm/clojure/lang/APersistentSet.java
patching file src/jvm/clojure/lang/APersistentVector.java
patching file src/jvm/clojure/lang/ASeq.java
patching file src/jvm/clojure/lang/LazySeq.java
patching file src/jvm/clojure/lang/PersistentQueue.java
patching file src/jvm/clojure/lang/RT.java
Show
Cosmin Stejerean added a comment - Odd, it seems to apply cleanly for me onto 485b6fc357157458f156bbba49f8e78c10c66c31 as well as 12f07da889819bc5613546ec223e97ac27c86dbf (current HEAD). 
$ git reset --hard HEAD
HEAD is now at 485b6fc [maven-release-plugin] prepare for next development iteration
$ git checkout 485b6fc357157458f156bbba49f8e78c10c66c31
HEAD is now at 485b6fc... [maven-release-plugin] prepare for next development iteration
$ patch -p1 < 0895-to-array-take-3.patch 
patching file src/jvm/clojure/lang/APersistentSet.java
patching file src/jvm/clojure/lang/APersistentVector.java
patching file src/jvm/clojure/lang/ASeq.java
patching file src/jvm/clojure/lang/LazySeq.java
patching file src/jvm/clojure/lang/PersistentQueue.java
patching file src/jvm/clojure/lang/RT.java
Hide
Permalink
Cosmin Stejerean added a comment - - edited

One more shot at getting a combined patch for this issue. 0001-CLJ-895.-Use-RT.seqToPassedArray-to-implement-Collec.patch contains the fix as well as the tests and should apply with no problems on top of e27a27d9a6dc3fd0d15f26a5076e2876007e0ae6

Show
Cosmin Stejerean added a comment - - edited One more shot at getting a combined patch for this issue. 0001-CLJ-895.-Use-RT.seqToPassedArray-to-implement-Collec.patch contains the fix as well as the tests and should apply with no problems on top of e27a27d9a6dc3fd0d15f26a5076e2876007e0ae6
Hide
Permalink
Stuart Halloway added a comment -

the take 4 patch is the same as previous, merely cleaned up to apply on master

Show
Stuart Halloway added a comment - the take 4 patch is the same as previous, merely cleaned up to apply on master
Hide
Permalink
Stuart Halloway added a comment -

Aargh. One last time with correct git usage. Ignore take-4, the take-5 patch applies on master.

Show
Stuart Halloway added a comment - Aargh. One last time with correct git usage. Ignore take-4, the take-5 patch applies on master.

People

Vote (1)
Watch (2)

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.