From 5ea7bd253eaecbdfd502c0204e13fbac7a16768e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pap=20L=C5=91rinc?= Date: Fri, 1 Jul 2016 15:29:27 +0300 Subject: [PATCH 1/2] Refactor: changed comparison methods for ObjectRange --- src/main/groovy/lang/ObjectRange.java | 35 ++++++++++++--------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/src/main/groovy/lang/ObjectRange.java b/src/main/groovy/lang/ObjectRange.java index d3581d46133..a55b1cba117 100644 --- a/src/main/groovy/lang/ObjectRange.java +++ b/src/main/groovy/lang/ObjectRange.java @@ -18,11 +18,8 @@ */ package groovy.lang; -import org.codehaus.groovy.runtime.DefaultGroovyMethods; import org.codehaus.groovy.runtime.InvokerHelper; import org.codehaus.groovy.runtime.IteratorClosureAdapter; -import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; -import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation; import java.math.BigDecimal; import java.math.BigInteger; @@ -30,6 +27,8 @@ import java.util.Iterator; import java.util.List; +import static org.codehaus.groovy.runtime.ScriptBytecodeAdapter.*; + /** * Represents an inclusive list of objects from a value to a value using * comparators. @@ -195,7 +194,7 @@ protected void checkBoundaryCompatibility() { private static boolean areReversed(Comparable from, Comparable to) { try { - return ScriptBytecodeAdapter.compareGreaterThan(from, to); + return compareGreaterThan(from, to); } catch (ClassCastException cce) { throw new IllegalArgumentException("Unable to create range due to incompatible types: " + from.getClass().getSimpleName() + ".." + to.getClass().getSimpleName() + " (possible missing brackets around range?)", cce); } @@ -214,8 +213,8 @@ public boolean equals(Object that) { public boolean equals(ObjectRange that) { return that != null && reverse == that.reverse - && DefaultTypeTransformation.compareEqual(from, that.from) - && DefaultTypeTransformation.compareEqual(to, that.to); + && compareEqual(from, that.from) + && compareEqual(to, that.to); } @Override @@ -259,16 +258,12 @@ public Comparable get(int index) { @Override public boolean containsWithinBounds(Object value) { if (value instanceof Comparable) { - final int result = compareTo(from, (Comparable) value); - return result == 0 || result < 0 && compareTo(to, (Comparable) value) >= 0; + final int result = compareTo(from, value); + return result == 0 || result < 0 && compareGreaterThanEqual(to, value); } return contains(value); } - protected int compareTo(Comparable first, Comparable second) { - return DefaultGroovyMethods.numberAwareCompareTo(first, second); - } - /** * protection against calls from Groovy */ @@ -385,7 +380,7 @@ public boolean contains(Object value) { } while (iter.hasNext()) { try { - if (DefaultTypeTransformation.compareEqual(value, iter.next())) return true; + if (compareEqual(value, iter.next())) return true; } catch (ClassCastException e) { return false; } @@ -395,7 +390,7 @@ public boolean contains(Object value) { @Override public void step(int step, Closure closure) { - if (step == 0 && compareTo(from, to) == 0) { + if (step == 0 && compareEqual(from, to)) { return; // from == to and step == 0, nothing to do, so return } final Iterator iter = new StepIterator(this, step); @@ -441,8 +436,8 @@ private static class StepIterator implements Iterator { private Comparable value; private boolean nextFetched = true; - private StepIterator(ObjectRange range, final int desiredStep) { - if (desiredStep == 0 && range.compareTo(range.getFrom(), range.getTo()) != 0) { + private StepIterator(ObjectRange range, int desiredStep) { + if (desiredStep == 0 && compareNotEqual(range.getFrom(), range.getTo())) { throw new GroovyRuntimeException("Infinite loop detected due to step size of 0"); } this.range = range; @@ -491,9 +486,9 @@ private Comparable peek() { for (int i = 0; i < step; i++) { peekValue = (Comparable) range.increment(peekValue); // handle back to beginning due to modulo incrementing - if (range.compareTo(peekValue, range.from) <= 0) return null; + if (compareLessThanEqual(peekValue, range.from)) return null; } - if (range.compareTo(peekValue, range.to) <= 0) { + if (compareLessThanEqual(peekValue, range.to)) { return peekValue; } } else { @@ -502,9 +497,9 @@ private Comparable peek() { for (int i = 0; i < positiveStep; i++) { peekValue = (Comparable) range.decrement(peekValue); // handle back to beginning due to modulo decrementing - if (range.compareTo(peekValue, range.to) >= 0) return null; + if (compareGreaterThanEqual(peekValue, range.to)) return null; } - if (range.compareTo(peekValue, range.from) >= 0) { + if (compareGreaterThanEqual(peekValue, range.from)) { return peekValue; } } From 1d82f35a7df66395d347fb2000dc3fb97e9f7159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pap=20L=C5=91rinc?= Date: Thu, 30 Jun 2016 02:05:03 +0300 Subject: [PATCH 2/2] Refactor: simplified ObjectRange and IntRange iterators and iterations * Made the fetching of the first element lazy also * Renamed the stepping iterator to make it obvious that it always calculates the current element, not the next - i.e. it's lazier --- src/main/groovy/lang/IntRange.java | 26 ++-- src/main/groovy/lang/ObjectRange.java | 170 +++++++++++++------------- 2 files changed, 95 insertions(+), 101 deletions(-) diff --git a/src/main/groovy/lang/IntRange.java b/src/main/groovy/lang/IntRange.java index 3855f436aab..9f6006e6208 100644 --- a/src/main/groovy/lang/IntRange.java +++ b/src/main/groovy/lang/IntRange.java @@ -385,26 +385,22 @@ public void step(int step, Closure closure) { return; // from == to and step == 0, nothing to do, so return } - if (isReverse()) { - step = -step; + final boolean isAscending; + if (step < 0) { + step *= -1; + isAscending = isReverse(); + } else { + isAscending = !isReverse(); } - if (step > 0) { - int value = getFrom(); - while (value <= getTo()) { + + final long from = getFrom().longValue(), to = getTo().longValue(); + if (isAscending) { + for (long value = from; value <= to; value += step) { closure.call(value); - if (((long) value + step) >= Integer.MAX_VALUE) { - break; - } - value = value + step; } } else { - int value = getTo(); - while (value >= getFrom()) { + for (long value = to; value >= from; value -= step) { closure.call(value); - if (((long) value + step) <= Integer.MIN_VALUE) { - break; - } - value = value + step; } } } diff --git a/src/main/groovy/lang/ObjectRange.java b/src/main/groovy/lang/ObjectRange.java index a55b1cba117..f19a9893a2b 100644 --- a/src/main/groovy/lang/ObjectRange.java +++ b/src/main/groovy/lang/ObjectRange.java @@ -237,16 +237,16 @@ public Comparable get(int index) { if (index < 0) { throw new IndexOutOfBoundsException("Index: " + index + " should not be negative"); } - final StepIterator iter = new StepIterator(this, 1); - Comparable value = iter.next(); - for (int i = 0; i < index; i++) { - if (!iter.hasNext()) { + int i = 0; + for (Iterator iter = new LazySteppingIterator(this, 1); ; i++) { + final Comparable next = iter.next(); + if (i == index) { + return next; + } else if (!iter.hasNext()) { throw new IndexOutOfBoundsException("Index: " + index + " is too big for range: " + this); } - value = iter.next(); } - return value; } /** @@ -303,8 +303,7 @@ public int size() { } } else { // let's brute-force calculate the size by iterating start to end - final Iterator iter = new StepIterator(this, 1); - while (iter.hasNext()) { + for (Iterator iter = new LazySteppingIterator(this, 1); iter.hasNext(); ) { tempsize++; // integer overflow if (tempsize < 0) { @@ -336,7 +335,7 @@ public List subList(int fromIndex, int toIndex) { // Performance detail: // not using get(fromIndex), get(toIndex) in the following to avoid stepping over elements twice - final Iterator iter = new StepIterator(this, 1); + final Iterator iter = new LazySteppingIterator(this, 1); Comparable toValue = iter.next(); int i = 0; @@ -374,15 +373,13 @@ public String inspect() { */ @Override public boolean contains(Object value) { - final Iterator iter = new StepIterator(this, 1); if (value == null) { return false; } - while (iter.hasNext()) { - try { - if (compareEqual(value, iter.next())) return true; - } catch (ClassCastException e) { - return false; + + for (Iterator iter = new LazySteppingIterator(this, 1); iter.hasNext(); ) { + if (compareEqual(value, iter.next())) { + return true; } } return false; @@ -393,8 +390,8 @@ public void step(int step, Closure closure) { if (step == 0 && compareEqual(from, to)) { return; // from == to and step == 0, nothing to do, so return } - final Iterator iter = new StepIterator(this, step); - while (iter.hasNext()) { + + for (Iterator iter = new LazySteppingIterator(this, step); iter.hasNext(); ) { closure.call(iter.next()); } } @@ -404,106 +401,89 @@ public void step(int step, Closure closure) { */ @Override public Iterator iterator() { - // non thread-safe iterator - final Iterator innerIterator = new StepIterator(this, 1); return new Iterator() { + private final Iterator delegate = new LazySteppingIterator(ObjectRange.this, 1); + @Override public synchronized boolean hasNext() { - return innerIterator.hasNext(); + return delegate.hasNext(); } @Override public synchronized Comparable next() { - return innerIterator.next(); + return delegate.next(); } @Override public synchronized void remove() { - innerIterator.remove(); + delegate.remove(); } }; } /** - * convenience class to serve in other methods. - * It's not thread-safe, and lazily produces the next element only on calls of hasNext() or next() + * Iterator with configurable step size + * It's not thread-safe, and lazily produces the current element (including the first one) + * only on hasNext() or next() calls. */ - private static class StepIterator implements Iterator { - // actual step, can be +1 when desired step is -1 and direction is from high to low + class LazySteppingIterator implements Iterator { + private final Range range; private final int step; - private final ObjectRange range; - private int index = -1; - private Comparable value; - private boolean nextFetched = true; + private final boolean isAscending; - private StepIterator(ObjectRange range, int desiredStep) { + private boolean isNextFetched = false; + private Comparable next = null; + + LazySteppingIterator(Range range, int desiredStep) { if (desiredStep == 0 && compareNotEqual(range.getFrom(), range.getTo())) { throw new GroovyRuntimeException("Infinite loop detected due to step size of 0"); } + this.range = range; - if (range.isReverse()) { - step = -desiredStep; + if (desiredStep < 0) { + step = -1 * desiredStep; + isAscending = range.isReverse(); } else { step = desiredStep; - } - if (step > 0) { - value = range.getFrom(); - } else { - value = range.getTo(); + isAscending = !range.isReverse(); } } @Override - public void remove() { - range.remove(index); + public boolean hasNext() { + fetchNextIfNeeded(); + return (next != null) && (isAscending ? compareLessThanEqual(next, range.getTo()) + : compareGreaterThanEqual(next, range.getFrom())); } @Override public Comparable next() { - // not thread safe - if (!nextFetched) { - value = peek(); - nextFetched = true; + if (!hasNext()) { + return null; } - nextFetched = false; - index++; - return value; - } - @Override - public boolean hasNext() { - // not thread safe - if (!nextFetched) { - value = peek(); - nextFetched = true; - } - return value != null; + fetchNextIfNeeded(); + isNextFetched = false; + return next; } - private Comparable peek() { - if (step > 0) { - Comparable peekValue = value; - for (int i = 0; i < step; i++) { - peekValue = (Comparable) range.increment(peekValue); - // handle back to beginning due to modulo incrementing - if (compareLessThanEqual(peekValue, range.from)) return null; - } - if (compareLessThanEqual(peekValue, range.to)) { - return peekValue; - } - } else { - final int positiveStep = -step; - Comparable peekValue = value; - for (int i = 0; i < positiveStep; i++) { - peekValue = (Comparable) range.decrement(peekValue); - // handle back to beginning due to modulo decrementing - if (compareGreaterThanEqual(peekValue, range.to)) return null; - } - if (compareGreaterThanEqual(peekValue, range.from)) { - return peekValue; + private void fetchNextIfNeeded() { + if (!isNextFetched) { + isNextFetched = true; + + if (next == null) { + next = isAscending ? range.getFrom() // make the first fetch lazy too + : range.getTo(); + } else { + next = isAscending ? increment(next, step) + : decrement(next, step); } } - return null; + } + + @Override + public void remove() { + throw new UnsupportedOperationException(); } } @@ -515,23 +495,41 @@ public List step(int step) { } /** - * Increments by one + * Increments by step size * * @param value the value to increment - * @return the incremented value + * @param step the value to increment by + * @return the incremented value or null, if there isn't any */ - protected Object increment(Object value) { - return InvokerHelper.invokeMethod(value, "next", null); + @SuppressWarnings("unchecked") + private Comparable increment(Comparable value, int step) { + for (int i = 0; i < step; i++) { + final Comparable next = (Comparable) InvokerHelper.invokeMethod(value, "next", null); + if (!compareGreaterThan(next, value)) /* e.g. `next` of the last element */ { + return null; + } + value = next; + } + return value; } /** - * Decrements by one + * Decrements by step size * * @param value the value to decrement - * @return the decremented value + * @param step the value to decrement by + * @return the decremented value or null, if there isn't any */ - protected Object decrement(Object value) { - return InvokerHelper.invokeMethod(value, "previous", null); + @SuppressWarnings("unchecked") + private Comparable decrement(Comparable value, int step) { + for (int i = 0; i < step; i++) { + final Comparable previous = (Comparable) InvokerHelper.invokeMethod(value, "previous", null); + if (!compareLessThan(previous, value)) /* e.g. `previous` of the first element */ { + return null; + } + value = previous; + } + return value; } /**