Skip to content

Commit

Permalink
Prevent various array methods from setting empty slots in sparse arra…
Browse files Browse the repository at this point in the history 
…ys to undefined.

Defining an array element as undefined yields a similar result as not defining it,
but the `in` operator will report the element as present. Affected methods are
reverse(), shift(), unshift(), slice(), splice(), and concat().

Also adds doctests for said methods on both sparse and dense arrays.
  • Loading branch information
@hns
hns committed Jul 28, 2011
1 parent 61bdb80 commit 702abfe
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 21 deletions.
64 changes: 43 additions & 21 deletions src/org/mozilla/javascript/NativeArray.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,16 @@ private static void setElem(Context cx, Scriptable target, long index,
}
}

// Similar as setElem(), but triggers deleteElem() if value is NOT_FOUND
private static void setRawElem(Context cx, Scriptable target, long index,
Object value) {
if (value == NOT_FOUND) {
deleteElem(target, index);
} else {
setElem(cx, target, index, value);
}
}

private static String toStringHelper(Context cx, Scriptable scope,
Scriptable thisObj,
boolean toSource, boolean toLocale)
Expand Down Expand Up @@ -937,10 +947,10 @@ private static Scriptable js_reverse(Context cx, Scriptable thisObj,
long half = len / 2;
for(long i=0; i < half; i++) {
long j = len - i - 1;
Object temp1 = getElem(cx, thisObj, i);
Object temp2 = getElem(cx, thisObj, j);
setElem(cx, thisObj, i, temp2);
setElem(cx, thisObj, j, temp1);
Object temp1 = getRawElem(thisObj, i);
Object temp2 = getRawElem(thisObj, j);
setRawElem(cx, thisObj, i, temp2);
setRawElem(cx, thisObj, j, temp1);
}
return thisObj;
}
Expand Down Expand Up @@ -1104,7 +1114,7 @@ private static Object js_shift(Context cx, Scriptable thisObj,
Object result = na.dense[0];
System.arraycopy(na.dense, 1, na.dense, 0, (int)na.length);
na.dense[(int)na.length] = NOT_FOUND;
return result;
return result == NOT_FOUND ? Undefined.instance : result;
}
}
Object result;
Expand All @@ -1122,8 +1132,8 @@ private static Object js_shift(Context cx, Scriptable thisObj,
*/
if (length > 0) {
for (i = 1; i <= length; i++) {
Object temp = getElem(cx, thisObj, i);
setElem(cx, thisObj, i - 1, temp);
Object temp = getRawElem(thisObj, i);
setRawElem(cx, thisObj, i - 1, temp);
}
}
// We don't need to delete the last property, because
Expand Down Expand Up @@ -1159,8 +1169,8 @@ private static Object js_unshift(Context cx, Scriptable thisObj,
/* Slide up the array to make room for args at the bottom */
if (length > 0) {
for (long last = length - 1; last >= 0; last--) {
Object temp = getElem(cx, thisObj, last);
setElem(cx, thisObj, last + argc, temp);
Object temp = getRawElem(thisObj, last);
setRawElem(cx, thisObj, last + argc, temp);
}
}

Expand Down Expand Up @@ -1242,9 +1252,13 @@ private static Object js_splice(Context cx, Scriptable scope,
} else {
Scriptable resultArray = cx.newArray(scope, 0);
for (long last = begin; last != end; last++) {
Object temp = getElem(cx, thisObj, last);
setElem(cx, resultArray, last - begin, temp);
Object temp = getRawElem(thisObj, last);
if (temp != NOT_FOUND) {
setElem(cx, resultArray, last - begin, temp);
}
}
// Need to set length for sparse result array
setLengthProperty(cx, resultArray, end - begin);
result = resultArray;
}
}
Expand Down Expand Up @@ -1277,13 +1291,13 @@ private static Object js_splice(Context cx, Scriptable scope,

if (delta > 0) {
for (long last = length - 1; last >= end; last--) {
Object temp = getElem(cx, thisObj, last);
setElem(cx, thisObj, last + delta, temp);
Object temp = getRawElem(thisObj, last);
setRawElem(cx, thisObj, last + delta, temp);
}
} else if (delta < 0) {
for (long last = end; last < length; last++) {
Object temp = getElem(cx, thisObj, last);
setElem(cx, thisObj, last + delta, temp);
Object temp = getRawElem(thisObj, last);
setRawElem(cx, thisObj, last + delta, temp);
}
}

Expand Down Expand Up @@ -1358,8 +1372,10 @@ private static Scriptable js_concat(Context cx, Scriptable scope,

// Copy from the target object into the result
for (slot = 0; slot < length; slot++) {
Object temp = getElem(cx, thisObj, slot);
setElem(cx, result, slot, temp);
Object temp = getRawElem(thisObj, slot);
if (temp != NOT_FOUND) {
setElem(cx, result, slot, temp);
}
}
} else {
setElem(cx, result, slot++, thisObj);
Expand All @@ -1375,13 +1391,16 @@ private static Scriptable js_concat(Context cx, Scriptable scope,
Scriptable arg = (Scriptable)args[i];
length = getLengthProperty(cx, arg);
for (long j = 0; j < length; j++, slot++) {
Object temp = getElem(cx, arg, j);
setElem(cx, result, slot, temp);
Object temp = getRawElem(arg, j);
if (temp != NOT_FOUND) {
setElem(cx, result, slot, temp);
}
}
} else {
setElem(cx, result, slot++, args[i]);
}
}
setLengthProperty(cx, result, slot);
return result;
}

Expand All @@ -1406,9 +1425,12 @@ private Scriptable js_slice(Context cx, Scriptable thisObj,
}

for (long slot = begin; slot < end; slot++) {
Object temp = getElem(cx, thisObj, slot);
setElem(cx, result, slot - begin, temp);
Object temp = getRawElem(thisObj, slot);
if (temp != NOT_FOUND) {
setElem(cx, result, slot - begin, temp);
}
}
setLengthProperty(cx, result, Math.max(0, end - begin));

return result;
}
Expand Down
69 changes: 69 additions & 0 deletions testsrc/doctests/array.dense.doctest
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// test various array methods with dense arrays
js> var x = []

js> x[10] = 1
1
js> x.length
11
js> 0 in x
false
js> x.shift()
js> x.length
10
js> 0 in x
false
js> x
,,,,,,,,,1
js> x.reverse()
1,,,,,,,,,
js> x.length
10
js> 9 in x
false
js> x.reverse()
,,,,,,,,,1
js> x.length
10
js> 5 in x
false
js> x.unshift(2)
11
js> x
2,,,,,,,,,,1
js> 9 in x
false
js> var r = x.splice(3, 4, 3, 4, 5)
js> r
,,,
js> r.length
4
js> 2 in r
false
js> x.length
10
js> x
2,,,3,4,5,,,,1
js> x[5]
5
js> 8 in x
false
js> var s = x.slice(6, 8);
js> s
,
js> s.length
2
js> 1 in s
false
js> var y = []
js> y[9] = 1
1
js> var z = y.concat(x)
js> z
,,,,,,,,,1,2,,,3,4,5,,,,1
js> z.length
20
js> 2 in z
false
js> 12 in z
false

69 changes: 69 additions & 0 deletions testsrc/doctests/array.sparse.doctest
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// test various array methods with sparse arrays
js> var x = []

js> x.length = 101
101
js> x[100] = 1
1
js> x.length
101
js> 0 in x
false
js> x.shift()
js> x.length
100
js> 0 in x
false
js> x
,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1
js> x.reverse()
1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
js> x.length
100
js> 10 in x
false
js> x.reverse()
,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1
js> x.length
100
js> 10 in x
false
js> x.unshift(2)
101
js> x
2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1
js> 10 in x
false
js> var r = x.splice(50, 5, 3, 4, 5)
js> r
,,,,
js> r.length
5
js> 3 in r
false
js> x.length
99
js> x
2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,3,4,5,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1
js> x[50]
3
js> 10 in x
false
js> var s = x.slice(60, 70);
js> s
,,,,,,,,,
js> s.length
10
js> 5 in s
false
js> var y = []
js> y.length = 100
100
js> var z = y.concat(x)
js> z.length
199
js> 30 in z
false
js> 130 in z
false

1 comment on commit 702abfe

@jdalton
Copy link

Choose a reason for hiding this comment

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

❤️

Please sign in to comment.