harmony-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jessewil...@apache.org
Subject svn commit: r888752 - in /harmony/enhanced/classlib/trunk/modules/luni/src: main/java/java/util/EnumSet.java main/java/java/util/HugeEnumSet.java main/java/java/util/MiniEnumSet.java test/api/common/org/apache/harmony/luni/tests/java/util/EnumSetTest.java
Date Wed, 09 Dec 2009 09:52:37 GMT
Author: jessewilson
Date: Wed Dec  9 09:52:37 2009
New Revision: 888752

URL: http://svn.apache.org/viewvc?rev=888752&view=rev
Log:
Give much needed attention to our EnumSet implementations.

Fix HugeEnumSet iterator.remove() bug
  Our iterator used a single index variable to track both the most recently returned element
and the next element. When these disagreed, remove was broken. Added a test case for this.

Fix HugeEnumSet concurrency bug
  The old code used of fields to track the intermediate values bitsIndex, elementIndex, and
oldIndex. This meant that concurrent reads were not thread safe, even if the EnumSet was never
modified after construction.

Fix retainAll bug
  Calling retainAll() on an empty enum set would return true when the passed in argument was
also an enum set.

Replace "collection.size() == 0" with "collection.isEmpty()"
  This is inefficient for ConcurrentHashMap, which must do a full iteration.

Remove no-op arrays.fill() in HugeEnumSet constructor
  Newly allocated arrays are initialized with 0s, this was a no-op.

Throw on clone failure
  Previously we swept an impossible clone failure under the rug by returning null. This can't
happen; we should throw AssertionErrors if it does.

Improve type safety
  Remove use of raw types. Move warning suppressions to single lines and document why the
suppressions are safe.

Increase internal implementation consistency
  The new structure of add/addAll, remove/removeAll, retainAll is consistent across all: for
each method, calculate the newBits. Compare old and new bits and if they disagree, update
size and return true.

Improve readability
  Flip "if (null == foo)" to "if (foo == null)" etc.

Investigate performance benefits of mod+divide vs. mask+shift
  On both DRLVM and the RI's VM, mod & divide by a constant factor performed no worse
than mask & shift. I'll investigate how Dalvik behaves on a device; I may later change
this code to use mask+shift.

Modified:
    harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/EnumSet.java
    harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/HugeEnumSet.java
    harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/MiniEnumSet.java
    harmony/enhanced/classlib/trunk/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/EnumSetTest.java

Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/EnumSet.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/EnumSet.java?rev=888752&r1=888751&r2=888752&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/EnumSet.java (original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/EnumSet.java Wed
Dec  9 09:52:37 2009
@@ -103,7 +103,7 @@
         if (c instanceof EnumSet) {
             return copyOf((EnumSet<E>) c);
         }
-        if (0 == c.size()) {
+        if (c.isEmpty()) {
             throw new IllegalArgumentException();
         }
         Iterator<E> iterator = c.iterator();
@@ -304,15 +304,13 @@
     @Override
     public EnumSet<E> clone() {
         try {
-            Object set = super.clone();
-            return (EnumSet<E>) set;
+            return (EnumSet<E>) super.clone();
         } catch (CloneNotSupportedException e) {
-            return null;
+            throw new AssertionError(e);
         }
     }
 
-    @SuppressWarnings("unchecked")
-    boolean isValidType(Class cls) {
+    boolean isValidType(Class<?> cls) {
         return cls == elementClass || cls.getSuperclass() == elementClass;
     }
 

Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/HugeEnumSet.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/HugeEnumSet.java?rev=888752&r1=888751&r2=888752&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/HugeEnumSet.java
(original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/HugeEnumSet.java
Wed Dec  9 09:52:37 2009
@@ -18,90 +18,95 @@
 
 
 /**
- * This is a concrete subclass of EnumSet designed specifically for enum type
- * with more than 64 elements.
- * 
+ * A concrete EnumSet for enums with more than 64 elements.
  */
 @SuppressWarnings("serial")
 final class HugeEnumSet<E extends Enum<E>> extends EnumSet<E> {
     
+    private static final int BIT_IN_LONG = 64;
+
     final private E[] enums;
     
     private long[] bits;
     
     private int size;
     
-    private static final int BIT_IN_LONG = 64;
-    
     HugeEnumSet(Class<E> elementType) {
         super(elementType);
         enums = elementType.getEnumConstants();
         bits = new long[(enums.length + BIT_IN_LONG - 1) / BIT_IN_LONG];
-        Arrays.fill(bits, 0);
     }
     
     private class HugeEnumSetIterator implements Iterator<E> {
 
-        private long[] unProcessedBits;
+        /**
+         * The bits yet to be returned for the long in bits[index]. As values from the current
index
+         * are returned, their bits are zeroed out. When this reaches zero, the index must
be
+         * incremented.
+         */
+        private long currentBits = bits[0];
 
-        private int bitsPosition = 0;
+        /**
+         * The index into HugeEnumSet.bits of the next value to return.
+         */
+        private int index;
 
-        /*
-         * Mask for current element.
+        /**
+         * The single bit of the next value to return.
          */
-        private long currentElementMask = 0;
+        private long mask;
 
-        boolean canProcess = true;
+        /**
+         * The candidate for removal. If null, no value may be removed.
+         */
+        private E last;
 
         private HugeEnumSetIterator() {
-            unProcessedBits = new long[bits.length];
-            System.arraycopy(bits, 0, unProcessedBits, 0, bits.length);
-            bitsPosition = unProcessedBits.length;
-            findNextNoneZeroPosition(0);
-            if (bitsPosition == unProcessedBits.length) {
-                canProcess = false;
-            }
+            computeNextElement();
         }
 
-        private void findNextNoneZeroPosition(int start) {
-            for (int i = start; i < unProcessedBits.length; i++) {
-                if (0 != bits[i]) {
-                    bitsPosition = i;
-                    break;
+        /**
+         * Assigns mask and index to the next available value, cycling currentBits as necessary.
+         */
+        void computeNextElement() {
+            while (true) {
+                if (currentBits != 0) {
+                    mask = currentBits & -currentBits; // the lowest 1 bit in currentBits
+                    return;
+                } else if (++index < bits.length) {
+                    currentBits = bits[index];
+                } else {
+                    mask = 0;
+                    return;
                 }
             }
         }
 
         public boolean hasNext() {
-            return canProcess;
+            return mask != 0;
         }
 
         public E next() {
-            if (!canProcess) {
+            if (mask == 0) {
                 throw new NoSuchElementException();
             }
-            currentElementMask = unProcessedBits[bitsPosition]
-                    & (-unProcessedBits[bitsPosition]);
-            unProcessedBits[bitsPosition] -= currentElementMask;
-            int index = Long.numberOfTrailingZeros(currentElementMask)
-                    + bitsPosition * BIT_IN_LONG;
-            if (0 == unProcessedBits[bitsPosition]) {
-                int oldBitsPosition = bitsPosition;
-                findNextNoneZeroPosition(bitsPosition + 1);
-                if (bitsPosition == oldBitsPosition) {
-                    canProcess = false;
-                }
-            }
-            return enums[index];
+
+            int ordinal = Long.numberOfTrailingZeros(mask) + index * BIT_IN_LONG;
+            last = enums[ordinal];
+
+            currentBits &= ~mask;
+            computeNextElement();
+
+            return last;
         }
 
         public void remove() {
-            if (currentElementMask == 0) {
+            if (last == null) {
                 throw new IllegalStateException();
             }
-            bits[bitsPosition] &= ~currentElementMask;
-            size--;
-            currentElementMask = 0;
+
+            HugeEnumSet.this.remove(last);
+            last = null;
         }
     }
     
@@ -110,38 +115,44 @@
         if (!isValidType(element.getDeclaringClass())) {
             throw new ClassCastException();
         }
-        calculateElementIndex(element);
 
-        bits[bitsIndex] |= (1l << elementInBits);
-        if (oldBits == bits[bitsIndex]) {
-            return false;
+        int ordinal = element.ordinal();
+        int index = ordinal / BIT_IN_LONG;
+        int inBits = ordinal % BIT_IN_LONG;
+        long oldBits = bits[index];
+        long newBits = oldBits | (1L << inBits);
+        if (oldBits != newBits) {
+            bits[index] = newBits;
+            size++;
+            return true;
         }
-        size++;
-        return true;
+        return false;
     }
     
     @Override
     public boolean addAll(Collection<? extends E> collection) {
-        if (0 == collection.size() || this == collection) {
+        if (collection.isEmpty() || collection == this) {
             return false;
         }
+
         if (collection instanceof EnumSet) {
-            EnumSet set = (EnumSet) collection;
+            EnumSet<?> set = (EnumSet<?>) collection;
             if (!isValidType(set.elementClass)) {
                 throw new ClassCastException();
             }
-            HugeEnumSet hugeSet = (HugeEnumSet) set;
-            boolean addSuccessful = false;
+
+            HugeEnumSet<E> hugeSet = (HugeEnumSet<E>) set;
+            boolean changed = false;
             for (int i = 0; i < bits.length; i++) {
-                oldBits = bits[i];
-                bits[i] |= hugeSet.bits[i];
-                if (oldBits != bits[i]) {
-                    addSuccessful = true;
-                    size = size - Long.bitCount(oldBits)
-                            + Long.bitCount(bits[i]);
+                long oldBits = bits[i];
+                long newBits = oldBits | hugeSet.bits[i];
+                if (oldBits != newBits) {
+                    bits[i] = newBits;
+                    size += Long.bitCount(newBits) - Long.bitCount(oldBits);
+                    changed = true;
                 }
             }
-            return addSuccessful;
+            return changed;
         }
         return super.addAll(collection);
     }
@@ -173,43 +184,39 @@
         }
     }
     
-    @SuppressWarnings("unchecked")
     @Override
     public boolean contains(Object object) {
-        if (null == object) {
+        if (object == null || !isValidType(object.getClass())) {
             return false;
         }
-        if (!isValidType(object.getClass())) {
-            return false;
-        }
-        calculateElementIndex((E)object);
-        return (bits[bitsIndex] & (1l << elementInBits)) != 0;
+
+        @SuppressWarnings("unchecked") // guarded by isValidType()
+        int ordinal = ((E) object).ordinal();
+        int index = ordinal / BIT_IN_LONG;
+        int inBits = ordinal % BIT_IN_LONG;
+        return (bits[index] & (1L << inBits)) != 0;
     }
     
     @Override
-    @SuppressWarnings("unchecked")
     public HugeEnumSet<E> clone() {
-        Object set = super.clone();
-        if (null != set) {
-            ((HugeEnumSet<E>) set).bits = bits.clone();
-            return (HugeEnumSet<E>) set;
-        }
-        return null;
+        HugeEnumSet<E> set = (HugeEnumSet<E>) super.clone();
+        set.bits = bits.clone();
+        return set;
     }
     
     @Override
     public boolean containsAll(Collection<?> collection) {
-        if (collection.size() == 0) {
+        if (collection.isEmpty()) {
             return true;
         }
         if (collection instanceof HugeEnumSet) {
-            HugeEnumSet set = (HugeEnumSet) collection;
-            if(isValidType(set.elementClass )) {
-                for(int i = 0; i < bits.length; i++) {
-                    if((bits[i] & set.bits[i]) != set.bits[i]){
+            HugeEnumSet<?> set = (HugeEnumSet<?>) collection;
+            if (isValidType(set.elementClass)) {
+                for (int i = 0; i < bits.length; i++) {
+                    long setBits = set.bits[i];
+                    if ((bits[i] & setBits) != setBits) {
                         return false;
                     }
-                    
                 }
                 return true;
             }
@@ -219,13 +226,13 @@
     
     @Override
     public boolean equals(Object object) {
-        if (null == object) {
+        if (object == null) {
             return false;
         }
         if (!isValidType(object.getClass())) {
             return super.equals(object);
         }
-        return Arrays.equals(bits, ((HugeEnumSet) object).bits);
+        return Arrays.equals(bits, ((HugeEnumSet<?>) object).bits);
     }
     
     @Override
@@ -235,38 +242,48 @@
     
     @Override
     public boolean remove(Object object) {
-        if (!contains(object)) {
+        if (object == null || !isValidType(object.getClass())) {
             return false;
         }
-        bits[bitsIndex] -= (1l << elementInBits);
-        size--;
-        return true;
+
+        @SuppressWarnings("unchecked") // guarded by isValidType()
+        int ordinal = ((E) object).ordinal();
+        int index = ordinal / BIT_IN_LONG;
+        int inBits = ordinal % BIT_IN_LONG;
+        long oldBits = bits[index];
+        long newBits = oldBits & ~(1L << inBits);
+        if (oldBits != newBits) {
+            bits[index] = newBits;
+            size--;
+            return true;
+        }
+        return false;
     }
     
     @Override
     public boolean removeAll(Collection<?> collection) {
-        if (0 == collection.size()) {
+        if (collection.isEmpty()) {
             return false;
         }
         
         if (collection instanceof EnumSet) {
-            EnumSet<E> set = (EnumSet<E>) collection;
+            EnumSet<?> set = (EnumSet<?>) collection;
             if (!isValidType(set.elementClass)) {
                 return false;
             }
-            boolean removeSuccessful = false;
-            long mask = 0;
+
+            HugeEnumSet<E> hugeSet = (HugeEnumSet<E>) set;
+            boolean changed = false;
             for (int i = 0; i < bits.length; i++) {
-                oldBits = bits[i];
-                mask = bits[i] & ((HugeEnumSet<E>) set).bits[i];
-                if (mask != 0) {
-                    bits[i] -= mask;
-                    size = (size - Long.bitCount(oldBits) + Long
-                            .bitCount(bits[i]));
-                    removeSuccessful = true;
+                long oldBits = bits[i];
+                long newBits = oldBits & ~hugeSet.bits[i];
+                if (oldBits != newBits) {
+                    bits[i] = newBits;
+                    size += Long.bitCount(newBits) - Long.bitCount(oldBits);
+                    changed = true;
                 }
             }
-            return removeSuccessful;
+            return changed;
         }
         return super.removeAll(collection);
     }
@@ -274,72 +291,65 @@
     @Override
     public boolean retainAll(Collection<?> collection) {
         if (collection instanceof EnumSet) {
-            EnumSet<E> set = (EnumSet<E>) collection;
+            EnumSet<?> set = (EnumSet<?>) collection;
             if (!isValidType(set.elementClass)) {
-                clear();
-                return true;
+                if (size > 0) {
+                    clear();
+                    return true;
+                } else {
+                    return false;
+                }
             }
 
-            boolean retainSuccessful = false;
-            oldBits = 0;
+            HugeEnumSet<E> hugeSet = (HugeEnumSet<E>) set;
+            boolean changed = false;
             for (int i = 0; i < bits.length; i++) {
-                oldBits = bits[i];
-                bits[i] &= ((HugeEnumSet<E>) set).bits[i];
-                if (oldBits != bits[i]) {
-                    size = size - Long.bitCount(oldBits)
-                            + Long.bitCount(bits[i]);
-                    retainSuccessful = true;
+                long oldBits = bits[i];
+                long newBits = oldBits & hugeSet.bits[i];
+                if (oldBits != newBits) {
+                    bits[i] = newBits;
+                    size += Long.bitCount(newBits) - Long.bitCount(oldBits);
+                    changed = true;
                 }
             }
-            return retainSuccessful;
+            return changed;
         }
         return super.retainAll(collection);
     }
     
     @Override
     void setRange(E start, E end) {
-        calculateElementIndex(start);
-        int startBitsIndex = bitsIndex;
-        int startElementInBits = elementInBits;
-        calculateElementIndex(end);
-        int endBitsIndex = bitsIndex;
-        int endElementInBits = elementInBits;
-        long range = 0;
-        if (startBitsIndex == endBitsIndex) {
-            range = (-1l >>> (BIT_IN_LONG -(endElementInBits - startElementInBits
+ 1))) << startElementInBits;
-            size -= Long.bitCount(bits[bitsIndex]);
-            bits[bitsIndex] |= range;
-            size += Long.bitCount(bits[bitsIndex]);
+        int startOrdinal = start.ordinal();
+        int startIndex = startOrdinal / BIT_IN_LONG;
+        int startInBits = startOrdinal % BIT_IN_LONG;
+
+        int endOrdinal = end.ordinal();
+        int endIndex = endOrdinal / BIT_IN_LONG;
+        int endInBits = endOrdinal % BIT_IN_LONG;
+
+        if (startIndex == endIndex) {
+            long range = (-1L >>> (BIT_IN_LONG -(endInBits - startInBits + 1)))
<< startInBits;
+            size -= Long.bitCount(bits[startIndex]);
+            bits[startIndex] |= range;
+            size += Long.bitCount(bits[startIndex]);
+
         } else {
-            range = (-1l >>> startElementInBits) << startElementInBits;
-            size -= Long.bitCount(bits[startBitsIndex]);
-            bits[startBitsIndex] |= range;
-            size += Long.bitCount(bits[startBitsIndex]);
-
-            // endElementInBits + 1 is the number of consecutive ones.
-            // 63 - endElementInBits is the following zeros of the right most one.
-            range = -1l >>> (BIT_IN_LONG - (endElementInBits + 1));
-            size -= Long.bitCount(bits[endBitsIndex]);
-            bits[endBitsIndex] |= range;
-            size += Long.bitCount(bits[endBitsIndex]);
-            for(int i = (startBitsIndex + 1); i <= (endBitsIndex - 1); i++) {
+            long range = (-1L >>> startInBits) << startInBits;
+            size -= Long.bitCount(bits[startIndex]);
+            bits[startIndex] |= range;
+            size += Long.bitCount(bits[startIndex]);
+
+            // endInBits + 1 is the number of consecutive ones.
+            // 63 - endInBits is the following zeros of the right most one.
+            range = -1L >>> (BIT_IN_LONG - (endInBits + 1));
+            size -= Long.bitCount(bits[endIndex]);
+            bits[endIndex] |= range;
+            size += Long.bitCount(bits[endIndex]);
+            for (int i = (startIndex + 1); i <= (endIndex - 1); i++) {
                 size -= Long.bitCount(bits[i]);
-                bits[i] = -1l;
+                bits[i] = -1L;
                 size += Long.bitCount(bits[i]);
             }
         }
     }
-    
-    private void calculateElementIndex(E element) {
-        int elementOrdinal = element.ordinal();
-        bitsIndex = elementOrdinal / BIT_IN_LONG;
-        elementInBits = elementOrdinal % BIT_IN_LONG;
-        oldBits = bits[bitsIndex];
-    }
-    
-    private int bitsIndex;
-
-    private int elementInBits;
-
-    private long oldBits;
 }

Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/MiniEnumSet.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/MiniEnumSet.java?rev=888752&r1=888751&r2=888752&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/MiniEnumSet.java
(original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/MiniEnumSet.java
Wed Dec  9 09:52:37 2009
@@ -18,9 +18,7 @@
 
 
 /**
- * This is a concrete subclass of EnumSet designed specifically for enum type
- * with less than or equal to 64 elements.
- * 
+ * A concrete EnumSet for enums with 64 or fewer elements.
  */
 @SuppressWarnings("serial")
 final class MiniEnumSet<E extends Enum<E>> extends EnumSet<E> {
@@ -39,45 +37,47 @@
     
     private class MiniEnumSetIterator implements Iterator<E> {
 
-        private long unProcessedBits;
-
-        /*
-         * Mask for current element.
+        /**
+         * The bits yet to be returned for bits. As values from the current index are returned,
+         * their bits are zeroed out.
          */
-        private long currentElementMask;
+        private long currentBits = bits;
 
-        private boolean canProcess = true;
+        /**
+         * The single bit of the next value to return.
+         */
+        private long mask = currentBits & -currentBits; // the lowest 1 bit in currentBits
 
-        private MiniEnumSetIterator() {
-            unProcessedBits = bits;
-            if (0 == unProcessedBits) {
-                canProcess = false;
-            }
-        }
+        /**
+         * The candidate for removal. If null, no value may be removed.
+         */
+        private E last;
 
         public boolean hasNext() {
-            return canProcess;
+            return mask != 0;
         }
 
         public E next() {
-            if (!canProcess) {
+            if (mask == 0) {
                 throw new NoSuchElementException();
             }
-            currentElementMask = unProcessedBits & (-unProcessedBits);
-            unProcessedBits -= currentElementMask;
-            if (0 == unProcessedBits) {
-                canProcess = false;
-            }
-            return enums[Long.numberOfTrailingZeros(currentElementMask)];
+
+            int ordinal = Long.numberOfTrailingZeros(mask);
+            last = enums[ordinal];
+
+            currentBits &= ~mask;
+            mask = currentBits & -currentBits; // the lowest 1 bit in currentBits
+
+            return last;
         }
 
         public void remove() {
-            if ( currentElementMask == 0 ) {
+            if (last == null) {
                 throw new IllegalStateException();
             }
-            bits &= ~currentElementMask;
-            size = Long.bitCount(bits);
-            currentElementMask = 0;
+
+            MiniEnumSet.this.remove(last);
+            last = null;
         }
     }
 
@@ -102,77 +102,83 @@
         if (!isValidType(element.getDeclaringClass())) {
             throw new ClassCastException();
         }
-        long mask = 1l << element.ordinal();
-        if ((bits & mask) == mask) {
-            return false;
-        }
-        bits |= mask;
 
-        size++;
-        return true;
+        long oldBits = bits;
+        long newBits = oldBits | (1L << element.ordinal());
+        if (oldBits != newBits) {
+            bits = newBits;
+            size++;
+            return true;
+        }
+        return false;
     }
     
     @Override
     public boolean addAll(Collection<? extends E> collection) {
-        if (0 == collection.size()) {
+        if (collection.isEmpty()) {
             return false;
         }
         if (collection instanceof EnumSet) {
-            EnumSet<?> set = (EnumSet)collection;
+            EnumSet<?> set = (EnumSet<?>) collection;
             if (!isValidType(set.elementClass)) {
                 throw new ClassCastException();
             }
+
             MiniEnumSet<?> miniSet = (MiniEnumSet<?>) set;
             long oldBits = bits;
-            bits |= miniSet.bits;
-            size = Long.bitCount(bits);
-            return (oldBits != bits);
+            long newBits = oldBits | miniSet.bits;
+            bits = newBits;
+            size = Long.bitCount(newBits);
+            return (oldBits != newBits);
         }
         return super.addAll(collection);
     }
     
     @Override
     public boolean contains(Object object) {
-        if (null == object) {
-            return false;
-        }
-        if (!isValidType(object.getClass())) {
+        if (object == null || !isValidType(object.getClass())) {
             return false;
         }
-        Enum<?> element = (Enum<?>) object;
+
+        @SuppressWarnings("unchecked") // guarded by isValidType()
+        Enum<E> element = (Enum<E>) object;
         int ordinal = element.ordinal();
-        return (bits & (1l << ordinal)) != 0;
+        return (bits & (1L << ordinal)) != 0;
     }
     
     @Override
     public boolean containsAll(Collection<?> collection) {
-        if (collection.size() == 0) {
+        if (collection.isEmpty()) {
             return true;
         }
         if (collection instanceof MiniEnumSet) {
             MiniEnumSet<?> set = (MiniEnumSet<?>) collection;
-            return isValidType(set.elementClass ) && ((bits & set.bits) == set.bits);
+            long setBits = set.bits;
+            return isValidType(set.elementClass) && ((bits & setBits) == setBits);
         }
         return !(collection instanceof EnumSet) && super.containsAll(collection);
 
     }
     
     @Override
     public boolean removeAll(Collection<?> collection) {
-        if (0 == collection.size()) {
+        if (collection.isEmpty()) {
             return false;
         }
         if (collection instanceof EnumSet) {
-            EnumSet<E> set = (EnumSet<E>) collection;
-            boolean removeSuccessful = false;
-            if (isValidType(set.elementClass)) {
-                long mask = bits & ((MiniEnumSet<E>) set).bits;
-                if (mask != 0) {
-                    bits -= mask;
-                    size = Long.bitCount(bits);
-                    removeSuccessful = true;
-                }
+            EnumSet<?> set = (EnumSet<?>) collection;
+            if (!isValidType(set.elementClass)) {
+                return false;
+            }
+
+            MiniEnumSet<E> miniSet = (MiniEnumSet<E>) set;
+            long oldBits = bits;
+            long newBits = oldBits & ~miniSet.bits;
+            if (oldBits != newBits) {
+                bits = newBits;
+                size = Long.bitCount(newBits);
+                return true;
             }
-            return removeSuccessful;
+            return false;
         }
         return super.removeAll(collection);
     }
@@ -180,33 +186,46 @@
     @Override
     public boolean retainAll(Collection<?> collection) {
         if (collection instanceof EnumSet) {
-            EnumSet<E> set = (EnumSet<E>) collection;
+            EnumSet<?> set = (EnumSet<?>) collection;
             if (!isValidType(set.elementClass)) {
-                clear();
-                return true;
+                if (size > 0) {
+                    clear();
+                    return true;
+                } else {
+                    return false;
+                }
             }
-            boolean retainSuccessful = false;
+
+            MiniEnumSet<E> miniSet = (MiniEnumSet<E>) set;
             long oldBits = bits;
-            bits &= ((MiniEnumSet<E>)set).bits;
-            if (oldBits != bits) {
-                size = Long.bitCount(bits);
-                retainSuccessful = true;
+            long newBits = oldBits & miniSet.bits;
+            if (oldBits != newBits) {
+                bits = newBits;
+                size = Long.bitCount(newBits);
+                return true;
             }
-            return retainSuccessful;
+            return false;
         }
         return super.retainAll(collection);
     }
     
     @Override
     public boolean remove(Object object) {
-        if (!contains(object)) {
+        if (object == null || !isValidType(object.getClass())) {
             return false;
         }
-        Enum<?> element = (Enum<?>) object;
+
+        @SuppressWarnings("unchecked") // guarded by isValidType() 
+        Enum<E> element = (Enum<E>) object;
         int ordinal = element.ordinal();
-        bits -= (1l << ordinal);
-        size--;
-        return true;
+        long oldBits = bits;
+        long newBits = oldBits & ~(1L << ordinal);
+        if (oldBits != newBits) {
+            bits = newBits;
+            size--;
+            return true;
+        }
+        return false;
     }
     
     @Override
@@ -214,18 +233,18 @@
         if (!(object instanceof EnumSet)) {
             return super.equals(object);
         }
-        EnumSet<?> set =(EnumSet<?>)object; 
-        if( !isValidType(set.elementClass) ) {
-            return size == 0 && set.size() == 0;
+        EnumSet<?> set =(EnumSet<?>) object;
+        if (!isValidType(set.elementClass)) {
+            return size == 0 && set.isEmpty();
         }
-        return bits == ((MiniEnumSet<?>)set).bits;
+        return bits == ((MiniEnumSet<?>) set).bits;
     }
     
     @Override
     void complement() {
-        if (0 != enums.length) {
+        if (enums.length != 0) {
             bits = ~bits;
-            bits &= (-1l >>> (MAX_ELEMENTS - enums.length));
+            bits &= (-1L >>> (MAX_ELEMENTS - enums.length));
             size = enums.length - size;
         }
     }
@@ -233,7 +252,7 @@
     @Override
     void setRange(E start, E end) {
         int length = end.ordinal() - start.ordinal() + 1;
-        long range = (-1l >>> (MAX_ELEMENTS - length)) << start.ordinal();
+        long range = (-1L >>> (MAX_ELEMENTS - length)) << start.ordinal();
         bits |= range;
         size = Long.bitCount(bits);
     }

Modified: harmony/enhanced/classlib/trunk/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/EnumSetTest.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/EnumSetTest.java?rev=888752&r1=888751&r2=888752&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/EnumSetTest.java
(original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/test/api/common/org/apache/harmony/luni/tests/java/util/EnumSetTest.java
Wed Dec  9 09:52:37 2009
@@ -137,6 +137,25 @@
         }
     }
 
+    public void testRemoveIteratorRemoveFromHugeEnumSet() {
+        EnumSet<HugeEnumCount> set = EnumSet.noneOf(HugeEnumCount.class);
+        set.add(HugeEnumCount.NO64);
+        set.add(HugeEnumCount.NO65);
+        set.add(HugeEnumCount.NO128);
+        Iterator<HugeEnumCount> iterator = set.iterator();
+        assertTrue(iterator.hasNext());
+        assertEquals(HugeEnumCount.NO64, iterator.next());
+        assertTrue(iterator.hasNext());
+        iterator.remove();
+        assertEquals(HugeEnumCount.NO65, iterator.next());
+        assertTrue(iterator.hasNext());
+        assertEquals(HugeEnumCount.NO128, iterator.next());
+        assertFalse(iterator.hasNext());
+        assertEquals(EnumSet.of(HugeEnumCount.NO65, HugeEnumCount.NO128), set);
+        iterator.remove();
+        assertEquals(EnumSet.of(HugeEnumCount.NO65), set);
+    }
+
     /**
      * @tests java.util.EnumSet#allOf(java.lang.Class)
      */
@@ -1220,21 +1239,21 @@
         Set<EnumWithInnerClass> setWithInnerClass = EnumSet
                 .allOf(EnumWithInnerClass.class);
         result = set.retainAll(setWithInnerClass);
-        assertTrue("Should return true", result); //$NON-NLS-1$
+        assertFalse("Should return false", result); //$NON-NLS-1$
         assertEquals("Size of set should be 0", 0, set.size()); //$NON-NLS-1$
 
         setWithInnerClass = EnumSet.noneOf(EnumWithInnerClass.class);
         result = set.retainAll(setWithInnerClass);
-        assertTrue("Should return true", result); //$NON-NLS-1$
+        assertFalse("Should return false", result); //$NON-NLS-1$
 
         Set<EmptyEnum> emptySet = EnumSet.allOf(EmptyEnum.class);
         result = set.retainAll(emptySet);
-        assertTrue("Should return true", result); //$NON-NLS-1$
+        assertFalse("Should return false", result); //$NON-NLS-1$
 
         Set<EnumWithAllInnerClass> setWithAllInnerClass = EnumSet
                 .allOf(EnumWithAllInnerClass.class);
         result = set.retainAll(setWithAllInnerClass);
-        assertTrue("Should return true", result); //$NON-NLS-1$
+        assertFalse("Should return false", result); //$NON-NLS-1$
 
         set.add(EnumFoo.a);
         result = set.retainAll(setWithInnerClass);
@@ -1314,17 +1333,17 @@
         Set<HugeEnumWithInnerClass> hugeSetWithInnerClass = EnumSet
                 .allOf(HugeEnumWithInnerClass.class);
         result = hugeSet.retainAll(hugeSetWithInnerClass);
-        assertTrue(result);
+        assertFalse(result);
         assertEquals(0, hugeSet.size());
 
         hugeSetWithInnerClass = EnumSet.noneOf(HugeEnumWithInnerClass.class);
         result = hugeSet.retainAll(hugeSetWithInnerClass);
-        assertTrue(result);
+        assertFalse(result);
 
         Set<HugeEnumWithInnerClass> hugeSetWithAllInnerClass = EnumSet
                 .allOf(HugeEnumWithInnerClass.class);
         result = hugeSet.retainAll(hugeSetWithAllInnerClass);
-        assertTrue(result);
+        assertFalse(result);
 
         hugeSet.add(HugeEnum.a);
         result = hugeSet.retainAll(hugeSetWithInnerClass);



Mime
View raw message