Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ADD: NativeArray.subList() #901

Merged
merged 4 commits into from
May 26, 2021
Merged

ADD: NativeArray.subList() #901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c089b84
ADD: NativeArray.subList()
rPraml May 20, 2021
a9b4499
spotless
rPraml May 20, 2021
e6ba640
Removing duplicate code and added text to IAE when list.length > Inte…
rPraml May 21, 2021
895aa34
Add: modCount to iterator/sublist
rPraml May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 57 additions & 23 deletions src/org/mozilla/javascript/NativeArray.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
import static org.mozilla.javascript.ScriptRuntimeES6.requireObjectCoercible;

import java.io.Serializable;
import java.util.AbstractList;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Comparator;
import java.util.ConcurrentModificationException;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
Expand Down Expand Up @@ -516,6 +518,7 @@ public void put(String id, Scriptable start, Object value) {
long index = toArrayIndex(id);
if (index >= length) {
length = index + 1;
modCount++;
denseOnly = false;
}
}
Expand Down Expand Up @@ -547,13 +550,17 @@ public void put(int index, Scriptable start, Object value) {
return;
} else if (index < dense.length) {
dense[index] = value;
if (this.length <= index) this.length = (long) index + 1;
if (this.length <= index) {
this.length = (long) index + 1;
this.modCount++;
}
return;
} else if (denseOnly
&& index < dense.length * GROW_FACTOR
&& ensureCapacity(index + 1)) {
dense[index] = value;
this.length = (long) index + 1;
this.modCount++;
return;
} else {
denseOnly = false;
Expand All @@ -565,6 +572,7 @@ && ensureCapacity(index + 1)) {
if (this.length <= index) {
// avoid overflowing index!
this.length = (long) index + 1;
this.modCount++;
}
}
}
Expand Down Expand Up @@ -687,6 +695,7 @@ protected void defineOwnProperty(
long index = toArrayIndex(id);
if (index >= length) {
length = index + 1;
modCount++;
}
super.defineOwnProperty(cx, id, desc, checkValid);
}
Expand Down Expand Up @@ -862,11 +871,13 @@ private void setLength(Object val) {
// downcast okay because denseOnly
Arrays.fill(dense, (int) longVal, dense.length, NOT_FOUND);
length = longVal;
modCount++;
return;
} else if (longVal < MAX_PRE_GROW_SIZE
&& longVal < (length * GROW_FACTOR)
&& ensureCapacity((int) longVal)) {
length = longVal;
modCount++;
return;
} else {
denseOnly = false;
Expand Down Expand Up @@ -897,6 +908,7 @@ && ensureCapacity((int) longVal)) {
}
}
length = longVal;
modCount++;
}

/* Support for generic Array-ish objects. Most of the Array
Expand Down Expand Up @@ -1248,6 +1260,7 @@ private static Object js_push(Context cx, Scriptable scope, Scriptable thisObj,
if (na.denseOnly && na.ensureCapacity((int) na.length + args.length)) {
for (int i = 0; i < args.length; i++) {
na.dense[(int) na.length++] = args[i];
na.modCount++;
}
return ScriptRuntime.wrapNumber(na.length);
}
Expand Down Expand Up @@ -1279,6 +1292,7 @@ private static Object js_pop(Context cx, Scriptable scope, Scriptable thisObj, O
NativeArray na = (NativeArray) o;
if (na.denseOnly && na.length > 0) {
na.length--;
na.modCount++;
result = na.dense[(int) na.length];
na.dense[(int) na.length] = NOT_FOUND;
return result;
Expand Down Expand Up @@ -1312,6 +1326,7 @@ private static Object js_shift(
NativeArray na = (NativeArray) o;
if (na.denseOnly && na.length > 0) {
na.length--;
na.modCount++;
Object result = na.dense[0];
System.arraycopy(na.dense, 1, na.dense, 0, (int) na.length);
na.dense[(int) na.length] = NOT_FOUND;
Expand Down Expand Up @@ -1359,6 +1374,7 @@ private static Object js_unshift(
na.dense[i] = args[i];
}
na.length += args.length;
na.modCount++;
return ScriptRuntime.wrapNumber(na.length);
}
}
Expand Down Expand Up @@ -1493,6 +1509,7 @@ private static Object js_splice(
Arrays.fill(na.dense, (int) (length + delta), (int) length, NOT_FOUND);
}
na.length = length + delta;
na.modCount++;
return result;
}

Expand Down Expand Up @@ -2100,11 +2117,7 @@ public Object[] toArray() {

@Override
public Object[] toArray(Object[] a) {
long longLen = length;
if (longLen > Integer.MAX_VALUE) {
throw new IllegalStateException();
}
int len = (int) longLen;
int len = size();
Object[] array =
a.length >= len
? a
Expand All @@ -2127,7 +2140,8 @@ public boolean containsAll(Collection c) {
public int size() {
long longLen = length;
if (longLen > Integer.MAX_VALUE) {
throw new IllegalStateException();
throw new IllegalStateException(
"list.length (" + length + ") exceeds Integer.MAX_VALUE");
}
return (int) longLen;
}
Expand Down Expand Up @@ -2158,11 +2172,7 @@ public Object get(int index) {

@Override
public int indexOf(Object o) {
long longLen = length;
if (longLen > Integer.MAX_VALUE) {
throw new IllegalStateException();
}
int len = (int) longLen;
int len = size();
if (o == null) {
for (int i = 0; i < len; i++) {
if (get(i) == null) {
Expand All @@ -2181,11 +2191,7 @@ public int indexOf(Object o) {

@Override
public int lastIndexOf(Object o) {
long longLen = length;
if (longLen > Integer.MAX_VALUE) {
throw new IllegalStateException();
}
int len = (int) longLen;
int len = size();
if (o == null) {
for (int i = len - 1; i >= 0; i--) {
if (get(i) == null) {
Expand Down Expand Up @@ -2214,11 +2220,7 @@ public ListIterator listIterator() {

@Override
public ListIterator listIterator(final int start) {
long longLen = length;
if (longLen > Integer.MAX_VALUE) {
throw new IllegalStateException();
}
final int len = (int) longLen;
final int len = size();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have this pattern at more places, maybe we can remove the duplicated code at all places

toArray()
indexOf()
lastIndexOf()

And maybe we can add some text to the illegal state exception thrown in the size() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: e6ba640


if (start < 0 || start > len) {
throw new IndexOutOfBoundsException("Index: " + start);
Expand All @@ -2227,6 +2229,7 @@ public ListIterator listIterator(final int start) {
return new ListIterator() {

int cursor = start;
int modCount = NativeArray.this.modCount;

@Override
public boolean hasNext() {
Expand All @@ -2235,6 +2238,7 @@ public boolean hasNext() {

@Override
public Object next() {
checkModCount(modCount);
if (cursor == len) {
throw new NoSuchElementException();
}
Expand All @@ -2248,6 +2252,7 @@ public boolean hasPrevious() {

@Override
public Object previous() {
checkModCount(modCount);
if (cursor == 0) {
throw new NoSuchElementException();
}
Expand Down Expand Up @@ -2333,7 +2338,33 @@ public Object remove(int index) {

@Override
public List subList(int fromIndex, int toIndex) {
throw new UnsupportedOperationException();
if (fromIndex < 0) throw new IndexOutOfBoundsException("fromIndex = " + fromIndex);
if (toIndex > size()) throw new IndexOutOfBoundsException("toIndex = " + toIndex);
if (fromIndex > toIndex)
throw new IllegalArgumentException(
"fromIndex(" + fromIndex + ") > toIndex(" + toIndex + ")");

return new AbstractList() {
private int modCount = NativeArray.this.modCount;

rPraml marked this conversation as resolved.
Show resolved Hide resolved
@Override
public Object get(int index) {
checkModCount(modCount);
return NativeArray.this.get(index + fromIndex);
}

@Override
public int size() {
checkModCount(modCount);
return toIndex - fromIndex;
}
};
}

private void checkModCount(int modCount) {
if (this.modCount != modCount) {
throw new ConcurrentModificationException();
}
}

@Override
Expand Down Expand Up @@ -2577,6 +2608,9 @@ protected int findPrototypeId(String s) {
/** Attributes of the array's length property */
private int lengthAttr = DONTENUM | PERMANENT;

/** modCount required for subList/iterators */
private transient int modCount;

/**
* Fast storage for dense arrays. Sparse arrays will use the superclass's hashtable storage
* scheme.
Expand Down
97 changes: 91 additions & 6 deletions testsrc/org/mozilla/javascript/tests/Bug466207Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,19 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.ConcurrentModificationException;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;

import junit.framework.TestCase;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.NativeArray;
import org.mozilla.javascript.ScriptableObject;

import junit.framework.TestCase;

/**
* See https://bugzilla.mozilla.org/show_bug.cgi?id=466207
*
* @author Hannes Wallnoefer
*/
public class Bug466207Test extends TestCase {
Expand All @@ -37,9 +38,14 @@ protected void setUp() {
// get a js object as map
Context context = Context.enter();
ScriptableObject scope = context.initStandardObjects();
list = (List<Object>) context.evaluateString(scope,
"(['a', true, new java.util.HashMap(), 42, 'a']);",
"testsrc", 1, null);
list =
(List<Object>)
context.evaluateString(
scope,
"(['a', true, new java.util.HashMap(), 42, 'a']);",
"testsrc",
1,
null);
Context.exit();
}

Expand Down Expand Up @@ -110,4 +116,83 @@ private void compareListIterators(ListIterator<Object> it1, ListIterator<Object>
assertFalse(it2.hasPrevious());
compareIterators(it1, it2);
}

public void testSublist() {
assertTrue(Arrays.equals(list.subList(0, 5).toArray(), reference.toArray()));
assertTrue(Arrays.equals(list.subList(2, 4).toArray(), reference.subList(2, 4).toArray()));
assertTrue(list.subList(0, 0).isEmpty());
assertTrue(list.subList(5, 5).isEmpty());
}

public void testSublistMod() {

List<Object> sl = reference.subList(2, 4);
reference.remove(0);
try {
sl.get(0);
fail("Exception expected");
} catch (ConcurrentModificationException cme) {

}
sl = list.subList(2, 4);
listPop();
assertEquals(4, list.size());
try {
sl.get(0);
fail("Exception expected");
} catch (ConcurrentModificationException cme) {

}
}

public void testIteratorMod() {

ListIterator<Object> iter = reference.listIterator();
reference.remove(0);
iter.previousIndex();
iter.nextIndex();
iter.hasNext();
try {
iter.next();
fail("Exception expected");
} catch (ConcurrentModificationException cme) {

}
iter = list.listIterator();
listPop();
assertEquals(4, list.size());
iter.previousIndex();
iter.nextIndex();
iter.hasNext();
try {
iter.next();
fail("Exception expected");
} catch (ConcurrentModificationException cme) {

}
}

private void listPop() {
Context context = Context.enter();
ScriptableObject scope = context.initStandardObjects();
scope.put("list", scope, list);
context.evaluateString(scope, "list.pop()", "testsrc", 1, null);
Context.exit();
}

public void testBigList() {
Context context = Context.enter();
ScriptableObject scope = context.initStandardObjects();
NativeArray array =
(NativeArray)
context.evaluateString(scope, "new Array(4294967295)", "testsrc", 1, null);
Context.exit();
assertEquals(4294967295L, array.getLength());
try {
array.size();
fail("Exception expected");
} catch (IllegalStateException e) {
assertEquals("list.length (4294967295) exceeds Integer.MAX_VALUE", e.getMessage());
}
}
}