harmony-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hinde...@apache.org
Subject svn commit: r983249 - in /harmony/enhanced/java/branches/java6: ./ classlib/ classlib/depends/libs/ classlib/modules/luni/src/main/java/java/util/ArrayList.java drlvm/ jdktools/
Date Sat, 07 Aug 2010 15:50:48 GMT
Author: hindessm
Date: Sat Aug  7 15:50:48 2010
New Revision: 983249

URL: http://svn.apache.org/viewvc?rev=983249&view=rev
Log:
Merge change from /harmony/enhanced/java/trunk@982377:

  r982377 | hindessm | 2010-08-04 20:35:59 +0100 (Wed, 04 Aug 2010) | 4 lines
  
  Remove unnecessary clone from addAll(...).
  Add review comments to save having to keep updating the patch and get
  more visibility of them.  Will remove them later.
  


Modified:
    harmony/enhanced/java/branches/java6/   (props changed)
    harmony/enhanced/java/branches/java6/classlib/   (props changed)
    harmony/enhanced/java/branches/java6/classlib/depends/libs/   (props changed)
    harmony/enhanced/java/branches/java6/classlib/modules/luni/src/main/java/java/util/ArrayList.java
    harmony/enhanced/java/branches/java6/drlvm/   (props changed)
    harmony/enhanced/java/branches/java6/jdktools/   (props changed)

Propchange: harmony/enhanced/java/branches/java6/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Aug  7 15:50:48 2010
@@ -1,4 +1,4 @@
 /harmony/enhanced/java/branches/mrh:935751-941490
-/harmony/enhanced/java/trunk:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250
+/harmony/enhanced/java/trunk:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250,982377
 /harmony/enhanced/trunk:476395-929252
 /incubator/harmony/enhanced/trunk:292550-476394

Propchange: harmony/enhanced/java/branches/java6/classlib/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Aug  7 15:50:48 2010
@@ -1,7 +1,7 @@
 /harmony/enhanced/classlib/trunk:713674-735919,765923-926091,926318-926838
 /harmony/enhanced/classlib/trunk/working_classlib:884014-884286
 /harmony/enhanced/java/branches/mrh/classlib:935751-941490
-/harmony/enhanced/java/trunk/classlib:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250
+/harmony/enhanced/java/trunk/classlib:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250,982377
 /harmony/enhanced/trunk/classlib:476395-929252
 /harmony/enhanced/trunk/working_classlib:476396-920147
 /incubator/harmony/enhanced/trunk/classlib:292550-476394

Propchange: harmony/enhanced/java/branches/java6/classlib/depends/libs/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Aug  7 15:50:48 2010
@@ -1,4 +1,4 @@
 /harmony/enhanced/classlib/trunk/depends/libs:544451-926091
-/harmony/enhanced/java/trunk/classlib/depends/libs:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250
+/harmony/enhanced/java/trunk/classlib/depends/libs:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250,982377
 /harmony/enhanced/trunk/classlib/depends/libs:476395-929252
 /incubator/harmony/enhanced/trunk/classlib/depends/libs:292550-476394

Modified: harmony/enhanced/java/branches/java6/classlib/modules/luni/src/main/java/java/util/ArrayList.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/java/branches/java6/classlib/modules/luni/src/main/java/java/util/ArrayList.java?rev=983249&r1=983248&r2=983249&view=diff
==============================================================================
--- harmony/enhanced/java/branches/java6/classlib/modules/luni/src/main/java/java/util/ArrayList.java
(original)
+++ harmony/enhanced/java/branches/java6/classlib/modules/luni/src/main/java/java/util/ArrayList.java
Sat Aug  7 15:50:48 2010
@@ -78,6 +78,10 @@ public class ArrayList<E> extends Abstra
         firstIndex = 0;
         Object[] objects = collection.toArray();
         int size = objects.length;
+
+        // REVIEW: Created 2 array copies of the original collection here
+        //         Could be better to use the collection iterator and
+        //         copy once?
         array = newElementArray(size + (size / 10));
         System.arraycopy(objects, 0, array, 0, size);
         lastIndex = size;
@@ -113,11 +117,15 @@ public class ArrayList<E> extends Abstra
                             Integer.valueOf(size)));
         }
         if (location == 0) {
+            // REVIEW: Does growAtFront() check the end to see if
+            //         shifting the array is possible? Same for
+            //         growAtEnd().
             if (firstIndex == 0) {
                 growAtFront(1);
             }
             array[--firstIndex] = object;
         } else if (location == size) {
+            // REVIEW: Why not just call add()? Matching RI behaviour?
             if (lastIndex == array.length) {
                 growAtEnd(1);
             }
@@ -127,6 +135,8 @@ public class ArrayList<E> extends Abstra
                 growForInsert(location, 1);
             } else if ((location < size / 2 && firstIndex > 0)
                     || lastIndex == array.length) {
+                // REVIEW: Why not evaluate (lastIndex==array.length)
+                //         first to save the divide?
                 System.arraycopy(array, firstIndex, array, --firstIndex,
                         location);
             } else {
@@ -175,6 +185,8 @@ public class ArrayList<E> extends Abstra
     @Override
     public boolean addAll(int location, Collection<? extends E> collection) {
         int size = lastIndex - firstIndex;
+        // REVIEW: Inconsistent exception case with
+        //         add(location,object) method
         if (location < 0 || location > size) {
             throw new IndexOutOfBoundsException(
                     // luni.0A=Index: {0}, Size: {1}
@@ -182,29 +194,40 @@ public class ArrayList<E> extends Abstra
                             Integer.valueOf(location),
                             Integer.valueOf(size)));
         }
-        if (this == collection) {
-            collection = (ArrayList)clone();
-        }
+
         Object[] dumparray = collection.toArray();
         int growSize = dumparray.length;
+        // REVIEW: Why do this check here rather than check
+        //         collection.size() earlier? RI behaviour?
         if (growSize == 0) {
             return false;
         }
 
+        // REVIEW: we use array.length - growSize in 3 places -
+        //         precalculate it?
         if (location == 0) {
             growAtFront(growSize);
             firstIndex -= growSize;
         } else if (location == size) {
+            // REVIEW: Don't need the above check as it can be no
+            //         other case. Make it just an else.
             if (lastIndex > array.length - growSize) {
                 growAtEnd(growSize);
             }
             lastIndex += growSize;
         } else { // must be case: (0 < location && location < size)
             if (array.length - size < growSize) {
+                // REVIEW: why grow growSize? Does growForInsert()
+                //         check we don't allocate too much?
                 growForInsert(location, growSize);
             } else if ((location < size / 2 && firstIndex > 0)
                     || lastIndex > array.length - growSize) {
+                // REVIEW: If condition above could be switched so
+                //         divide is done 2nd, same as add()
                 int newFirst = firstIndex - growSize;
+                // REVIEW: Have we optimised for the case where there
+                //         is enough space at the end for growSize
+                //         rather than doing 2 array copies?
                 if (newFirst < 0) {
                     int index = location + firstIndex;
                     System.arraycopy(array, index, array, index - newFirst,
@@ -260,7 +283,12 @@ public class ArrayList<E> extends Abstra
     @Override
     public void clear() {
         if (firstIndex != lastIndex) {
+            // REVIEW: Should we use Arrays.fill() instead of just
+            //         allocating a new array?  Should we use the same
+            //         sized array?
             Arrays.fill(array, firstIndex, lastIndex, null);
+            // REVIEW: Should the indexes point into the middle of the
+            //         array rather than 0?
             firstIndex = lastIndex = 0;
             modCount++;
         }
@@ -320,6 +348,8 @@ public class ArrayList<E> extends Abstra
      */
     public void ensureCapacity(int minimumCapacity) {
         if (array.length < minimumCapacity) {
+            // REVIEW: Why do we check the firstIndex first? Growing
+            //         the end makes more sense
             if (firstIndex > 0) {
                 growAtFront(minimumCapacity - array.length);
             } else {
@@ -344,7 +374,11 @@ public class ArrayList<E> extends Abstra
     private void growAtEnd(int required) {
         int size = lastIndex - firstIndex;
         if (firstIndex >= required - (array.length - lastIndex)) {
+            // REVIEW: Should use size! We don't seem to need newLast
+            //         - just use size calculated above
             int newLast = lastIndex - firstIndex;
+            // REVIEW: Why not just a !=0 check here? And size cannot
+            //         be 0 unless required is 0, which does not happen
             if (size > 0) {
                 System.arraycopy(array, firstIndex, array, 0, size);
                 int start = newLast < firstIndex ? firstIndex : newLast;
@@ -353,6 +387,8 @@ public class ArrayList<E> extends Abstra
             firstIndex = 0;
             lastIndex = newLast;
         } else {
+            // REVIEW: If size is 0?
+            //         Does size/2 seems a little high!
             int increment = size / 2;
             if (required > increment) {
                 increment = required;
@@ -361,6 +397,8 @@ public class ArrayList<E> extends Abstra
                 increment = 12;
             }
             E[] newArray = newElementArray(size + increment);
+            // REVIEW: Just check size !=0? same as earlier check -
+            //         can be pulled out to earlier in the method?
             if (size > 0) {
                 System.arraycopy(array, firstIndex, newArray, 0, size);
                 firstIndex = 0;
@@ -372,10 +410,15 @@ public class ArrayList<E> extends Abstra
 
     private void growAtFront(int required) {
         int size = lastIndex - firstIndex;
+        // REVIEW: replace lastIndex - firstIndex with size ... in all
+        //         methods that have it
         if (array.length - lastIndex + firstIndex >= required) {
             int newFirst = array.length - size;
+            // REVIEW: as growAtEnd, why not move size == 0 out as
+            //         special case
             if (size > 0) {
                 System.arraycopy(array, firstIndex, array, newFirst, size);
+                // REVIEW: firstIndex + size == lastIndex ??
                 int length = firstIndex + size > newFirst ? newFirst
                         : firstIndex + size;
                 Arrays.fill(array, firstIndex, length, null);
@@ -402,6 +445,9 @@ public class ArrayList<E> extends Abstra
     }
 
     private void growForInsert(int location, int required) {
+        // REVIEW: we grow too quickly because we are called with the
+        //         size of the new collection to add without taking in
+        //         to account the free space we already have
         int size = lastIndex - firstIndex;
         int increment = size / 2;
         if (required > increment) {
@@ -411,6 +457,8 @@ public class ArrayList<E> extends Abstra
             increment = 12;
         }
         E[] newArray = newElementArray(size + increment);
+        // REVIEW: biased towards leaving space at the beginning?
+        //         perhaps newFirst should be (increment-required)/2?
         int newFirst = increment - required;
         // Copy elements after location to the new array skipping inserted
         // elements
@@ -426,6 +474,7 @@ public class ArrayList<E> extends Abstra
 
     @Override
     public int indexOf(Object object) {
+        // REVIEW: should contains call this method?
         if (object != null) {
             for (int i = firstIndex; i < lastIndex; i++) {
                 if (object.equals(array[i])) {
@@ -489,6 +538,9 @@ public class ArrayList<E> extends Abstra
             result = array[--lastIndex];
             array[lastIndex] = null;
         } else if (location == 0) {
+            // REVIEW: this if test is simpler why isn't it first?
+            //         (this moved up one place during the exception
+            //         change but it still probably should move up one more)
             result = array[firstIndex];
             array[firstIndex++] = null;
         } else {
@@ -504,6 +556,8 @@ public class ArrayList<E> extends Abstra
                 array[--lastIndex] = null;
             }
         }
+        // REVIEW: we can move this to the first if case since it
+        //         can only occur when size==1
         if (firstIndex == lastIndex) {
             firstIndex = lastIndex = 0;
         }
@@ -535,13 +589,16 @@ public class ArrayList<E> extends Abstra
      */
     @Override
     protected void removeRange(int start, int end) {
+        // REVIEW: does RI call this from remove(location)
         int size = lastIndex - firstIndex;
         if (start < 0) {
+            // REVIEW: message should indicate which index is out of range
             throw new IndexOutOfBoundsException(
                     // luni.0B=Array index out of range: {0}
                     Messages.getString("luni.0B", //$NON-NLS-1$
                                        Integer.valueOf(start)));
         } else if (end > size) {
+            // REVIEW: message should indicate which index is out of range
             throw new IndexOutOfBoundsException(
                     // luni.0A=Index: {0}, Size: {1}
                     Messages.getString("luni.0A", //$NON-NLS-1$
@@ -563,6 +620,7 @@ public class ArrayList<E> extends Abstra
             Arrays.fill(array, firstIndex, firstIndex + end, null);
             firstIndex += end;
         } else {
+            // REVIEW: should this optimize to do the smallest copy?
             System.arraycopy(array, firstIndex + end, array, firstIndex
                              + start, size - end);
             int newLast = lastIndex + start - end;
@@ -606,6 +664,7 @@ public class ArrayList<E> extends Abstra
      */
     @Override
     public int size() {
+        // REVIEW: We should track firstIndex and size rather than lastIndex
         return lastIndex - firstIndex;
     }
 
@@ -648,6 +707,8 @@ public class ArrayList<E> extends Abstra
         }
         System.arraycopy(array, firstIndex, contents, 0, size);
         if (size < contents.length) {
+            // REVIEW: do we use this incorrectly - i.e. do we null
+            //         the rest out?
             contents[size] = null;
         }
         return contents;

Propchange: harmony/enhanced/java/branches/java6/drlvm/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Aug  7 15:50:48 2010
@@ -1,5 +1,5 @@
 /harmony/enhanced/java/branches/mrh/drlvm:935751-941490
-/harmony/enhanced/java/trunk/drlvm:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250
+/harmony/enhanced/java/trunk/drlvm:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250,982377
 /harmony/enhanced/trunk/drlvm:476395-929252
 /harmony/enhanced/trunk/working_vm:476396-920147
 /incubator/harmony/enhanced/trunk/drlvm:292550-476394

Propchange: harmony/enhanced/java/branches/java6/jdktools/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Aug  7 15:50:48 2010
@@ -1,4 +1,4 @@
-/harmony/enhanced/java/trunk/jdktools:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250
+/harmony/enhanced/java/trunk/jdktools:929253-979569,979593,979613,979615,979647,979659,979682,979897,980326,980632,981356,981763,981811,981820,982146,982148,982183,982250,982377
 /harmony/enhanced/jdktools/trunk:630107-925933
 /harmony/enhanced/trunk/jdktools:476395-929252
 /harmony/enhanced/trunk/working_jdktools:476396-920147



Mime
View raw message