-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Skylark] Make range function lazy. #5240
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; | ||
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; | ||
import com.google.devtools.build.lib.syntax.SkylarkMutable.BaseMutableList; | ||
import java.util.AbstractList; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
@@ -116,8 +117,10 @@ public String toString() { | |
@Override | ||
public boolean equals(Object object) { | ||
return (this == object) | ||
|| ((object != null) && (this.getClass() == object.getClass()) | ||
&& getContentsUnsafe().equals(((SkylarkList) object).getContentsUnsafe())); | ||
|| ((object != null) && (this.getClass() == object.getClass() | ||
|| this instanceof MutableListLike && object instanceof MutableListLike | ||
|| this instanceof TupleLike && object instanceof TupleLike) | ||
&& getContentsUnsafe().equals(((SkylarkList) object).getContentsUnsafe())); | ||
} | ||
|
||
@Override | ||
|
@@ -201,6 +204,123 @@ public static <E> SkylarkList<E> createImmutable(Iterable<? extends E> contents) | |
return MutableList.copyOf(Mutability.IMMUTABLE, contents); | ||
} | ||
|
||
/** An interface for classes that can be converted to {@link MutableList}. */ | ||
public interface MutableListLike<E> { | ||
MutableList<E> toMutableList(); | ||
} | ||
|
||
/** An interface for classes that can be converted to {@link Tuple}. */ | ||
public interface TupleLike<E> { | ||
Tuple<E> toTuple(); | ||
} | ||
|
||
/** | ||
* A sequence returned by the range function invocation. | ||
* | ||
* Instead of eagerly allocating an array with all elements of the sequence, this class uses | ||
* simple math to compute a value at each index. This is particularly useful when range is huge | ||
* or only a few elements from it are used. | ||
*/ | ||
public static final class RangeList extends SkylarkList<Integer> implements | ||
MutableListLike<Integer>, TupleLike<Integer> { | ||
|
||
/** Provides access to range elements based on their index. */ | ||
private static class RangeListView extends AbstractList<Integer> { | ||
private static int computeSize(int start, int stop, int step) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you're There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've decided to implement it similarly to https://github.com/python/cpython/blob/09bb918a61031377d720f1a0fa1fe53c962791b6/Objects/rangeobject.c#L144-L210 to make it easier to port CPython behavior |
||
final int length = Math.abs(stop - start); | ||
final int absolute_step = Math.abs(step); | ||
// round up (length / absolute_step) without using floats | ||
return 1 + ((length - 1) / absolute_step); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
private final int start; | ||
private final int stop; | ||
private final int step; | ||
private final int size; | ||
|
||
private RangeListView(int start, int stop, int step) { | ||
this.start = start; | ||
this.stop = stop; | ||
this.step = step; | ||
this.size = computeSize(start, stop, step); | ||
} | ||
|
||
@Override | ||
public Integer get(int index) { | ||
int value = start + step * index; | ||
if ((step > 0 && value > stop) || (step < 0 && value < stop)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If stop is exclusive, shouldn't it be if Then again, why do the computation on values at all? Why not just test if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great point! I didn't realize that index that is passed here is already normalized |
||
throw new ArrayIndexOutOfBoundsException(index); | ||
} | ||
return value; | ||
} | ||
|
||
@Override | ||
public int size() { | ||
return size; | ||
} | ||
} | ||
|
||
private final AbstractList<Integer> contents; | ||
|
||
private RangeList(int start, int stop, int step) { | ||
this.contents = new RangeListView(start, stop, step); | ||
} | ||
|
||
@Override | ||
public boolean isTuple() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public ImmutableList<Integer> getImmutableList() { | ||
return ImmutableList.copyOf(contents); | ||
} | ||
|
||
@Override | ||
public SkylarkList<Integer> getSlice(Object start, Object end, Object step, Location loc, | ||
Mutability mutability) throws EvalException { | ||
List<Integer> sliceIndices = EvalUtils.getSliceIndices(start, end, step, this.size(), loc); | ||
ImmutableList.Builder<Integer> builder = ImmutableList.builderWithExpectedSize(sliceIndices.size()); | ||
for (int pos : sliceIndices) { | ||
builder.add(this.get(pos)); | ||
} | ||
return MutableList.createImmutable(builder.build()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe implement it lazily? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. absolutely, I meant to leave a TODO for this though, since I didn't want to complicate this change even more. Also, I believe that all slice implementations should be made lazy and more efficient, since currently MutableList and Tuple both create news collections, which can be avoided. I'll try to create a new abstraction for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A TODO is fine for me.
|
||
} | ||
|
||
@Override | ||
public SkylarkList<Integer> repeat(int times, Mutability mutability) { | ||
ImmutableList.Builder<Integer> builder = ImmutableList.builderWithExpectedSize(this.size() * times); | ||
for (int i = 0; i < times; i++) { | ||
builder.addAll(this); | ||
} | ||
return MutableList.createImmutable(builder.build()); | ||
} | ||
|
||
@Override | ||
protected List<Integer> getContentsUnsafe() { | ||
return contents; | ||
} | ||
|
||
@Override | ||
public Mutability mutability() { | ||
return Mutability.IMMUTABLE; | ||
} | ||
|
||
@Override | ||
public MutableList<Integer> toMutableList() { | ||
return MutableList.copyOf(Mutability.IMMUTABLE, contents); | ||
} | ||
|
||
@Override | ||
public Tuple<Integer> toTuple() { | ||
return Tuple.copyOf(contents); | ||
} | ||
|
||
public static RangeList of(int start, int stop, int step) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. javadoc |
||
return new RangeList(start, stop, step); | ||
} | ||
} | ||
|
||
/** | ||
* A Skylark list, i.e., the value represented by {@code [1, 2, 3]}. Lists are mutable datatypes. | ||
*/ | ||
|
@@ -222,7 +342,7 @@ public static <E> SkylarkList<E> createImmutable(Iterable<? extends E> contents) | |
+ "['a', 'b', 'c', 'd'][3:0:-1] # ['d', 'c', 'b']</pre>" | ||
+ "Lists are mutable, as in Python." | ||
) | ||
public static final class MutableList<E> extends SkylarkList<E> { | ||
public static final class MutableList<E> extends SkylarkList<E> implements MutableListLike<E> { | ||
|
||
private final ArrayList<E> contents; | ||
|
||
|
@@ -565,6 +685,11 @@ public Object pop(Object i, Location loc, Environment env) | |
remove(index, loc, env.mutability()); | ||
return result; | ||
} | ||
|
||
@Override | ||
public MutableList<E> toMutableList() { | ||
return this; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -589,7 +714,7 @@ public Object pop(Object i, Location loc, Environment env) | |
+ "('a', 'b', 'c', 'd')[3:0:-1] # ('d', 'c', 'b')</pre>" | ||
+ "Tuples are immutable, therefore <code>x[1] = \"a\"</code> is not supported." | ||
) | ||
public static final class Tuple<E> extends SkylarkList<E> { | ||
public static final class Tuple<E> extends SkylarkList<E> implements TupleLike<E> { | ||
|
||
private final ImmutableList<E> contents; | ||
|
||
|
@@ -698,5 +823,10 @@ public Tuple<E> repeat(int times, Mutability mutability) { | |
} | ||
return copyOf(builder.build()); | ||
} | ||
|
||
@Override | ||
public Tuple<E> toTuple() { | ||
return this; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -486,6 +486,7 @@ public void testRange() throws Exception { | |
.testStatement("str(range(5, 0, -1))", "[5, 4, 3, 2, 1]") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test for mult, which is also now disabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought I had one... I guess I was wrong... :) |
||
.testStatement("str(range(5, 0, -10))", "[5]") | ||
.testStatement("str(range(0, -3, -2))", "[0, -2]") | ||
.testStatement("str(range(3)[1:2])", "[1]") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add more tests combining range and slicing |
||
.testIfErrorContains("step cannot be 0", "range(2, 3, 0)"); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Shouldn't this be ok albeit degenerate?
If you do this for step > 0, you probably also want to set stop = Math.min(start, stop) when step < 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after playing with Python interpreter I realized that Python actually allows this and displays the range the way it was constructed, so this change is not good. I'm going to remove it.