<< Back to previous view

[CLJ-895] Collection.toArray implementations do not conform to Java API docs Created: 10/Dec/11  Updated: 01/Mar/13  Resolved: 19/Feb/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-895.-Use-RT.seqToPassedArray-to-implement-Collec.patch     Text File 0895-take-4.patch     Text File 0895-take-5.patch     Text File 0895-to-array.patch     Text File 0895-to-array-take-2.patch    
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.

 Comments   
Comment by Stuart Halloway [ 10/Dec/11 5:35 PM ]

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

Comment by Cosmin Stejerean [ 10/Dec/11 10:44 PM ]

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.

Comment by Stuart Halloway [ 11/Dec/11 6:53 AM ]

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

Comment by Cosmin Stejerean [ 11/Dec/11 10:17 PM ]

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

Comment by Cosmin Stejerean [ 12/Dec/11 1:56 AM ]

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.

Comment by Stuart Sierra [ 16/Dec/11 9:30 AM ]

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

Comment by Cosmin Stejerean [ 16/Dec/11 4:20 PM ]

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
Comment by Cosmin Stejerean [ 03/Feb/12 6:52 PM ]

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

Comment by Stuart Halloway [ 19/Feb/12 6:48 PM ]

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

Comment by Stuart Halloway [ 19/Feb/12 7:17 PM ]

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

Generated at Tue May 21 22:46:32 CDT 2013 using JIRA 4.4#649-r158309.