Reviewers: cbruni,
Message:
I am a little hesitant to push ahead with this strategy until we have
benchmarks
to show it doesn't give a regression. In particular, I wanted to test the
performance of this patch against the baseline, and also against just not
having
the builtin at all and going straight to the JS version. cbruni, do you
have any
tests that could be used for this purpose?
Description:
Fix @@isConcatSpreadable in ArrayConcat builtin
The ArrayConcat builtin didn't respect @@isConcatSpreadable, which,
surprisingly, is observable if you modify an Array's
Symbol.isConcatSpreadable
property. This patch bails out of the ArrayConcat builtin if any argument's
Symbol.isConcatSpreadable property is set to false, but continues otherwise,
meeting the spec's requirements.
BUG=v8:4317
LOG=N
Please review this at https://codereview.chromium.org/1247243003/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+37, -19 lines):
M src/builtins.cc
M src/elements.h
M src/elements.cc
M src/runtime/runtime-array.cc
M test/mjsunit/harmony/array-concat.js
Index: src/builtins.cc
diff --git a/src/builtins.cc b/src/builtins.cc
index
69c2871650e87b16abea5b6c8abd3f8522247236..ccdec3cfd385c7bc5dfbc61d9187ab94757e4f44
100644
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -944,9 +944,14 @@ BUILTIN(ArrayConcat) {
bool is_holey = false;
for (int i = 0; i < n_arguments; i++) {
Object* arg = args[i];
+ bool fallback = false;
PrototypeIterator iter(isolate, arg);
+ if (FLAG_harmony_concat_spreadable) {
+ Handle<Object> obj(arg, isolate);
+ fallback = !IsConcatSpreadable(isolate, obj);
+ }
if (!arg->IsJSArray() || !JSArray::cast(arg)->HasFastElements() ||
- iter.GetCurrent() != array_proto) {
+ iter.GetCurrent() != array_proto || fallback) {
AllowHeapAllocation allow_allocation;
return CallJsBuiltin(isolate, "$arrayConcat", args);
}
Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
index
85eb3f266ecdb135e0ac8081d2b0d08bdd330477..917a31d0b5c85f3a1d493f820ccea2e0b6c3780f
100644
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -1946,5 +1946,23 @@ void ElementsAccessor::TearDown() {
ElementsAccessor** ElementsAccessor::elements_accessors_ = NULL;
+
+
+bool IsConcatSpreadable(Isolate* isolate, Handle<Object> obj) {
+ HandleScope handle_scope(isolate);
+ if (!obj->IsSpecObject()) return false;
+ if (FLAG_harmony_concat_spreadable) {
+ Handle<Symbol> key(isolate->factory()->is_concat_spreadable_symbol());
+ Handle<Object> value;
+ MaybeHandle<Object> maybeValue =
+ i::Runtime::GetObjectProperty(isolate, obj, key);
+ if (maybeValue.ToHandle(&value)) {
+ if (!value->IsUndefined()) {
+ return value->BooleanValue();
+ }
+ }
+ }
+ return obj->IsJSArray();
+}
} // namespace internal
} // namespace v8
Index: src/elements.h
diff --git a/src/elements.h b/src/elements.h
index
511b636c3de8082ed2c5dfe16251e7d5e8da08c1..df283889bf9ad985e83b2aaf6ebfecc5b6d1d95c
100644
--- a/src/elements.h
+++ b/src/elements.h
@@ -156,6 +156,8 @@ MUST_USE_RESULT MaybeHandle<Object>
ArrayConstructInitializeElements(
Handle<JSArray> array,
Arguments* args);
+bool IsConcatSpreadable(Isolate* isolate, Handle<Object> obj);
+
} } // namespace v8::internal
#endif // V8_ELEMENTS_H_
Index: src/runtime/runtime-array.cc
diff --git a/src/runtime/runtime-array.cc b/src/runtime/runtime-array.cc
index
af2d4c54813104a5e9566293951e9c488822b9fd..9cdf9237e0c52ace1caa54d88d082ed22966d360
100644
--- a/src/runtime/runtime-array.cc
+++ b/src/runtime/runtime-array.cc
@@ -726,24 +726,6 @@ static bool IterateElements(Isolate* isolate,
Handle<JSObject> receiver,
}
-static bool IsConcatSpreadable(Isolate* isolate, Handle<Object> obj) {
- HandleScope handle_scope(isolate);
- if (!obj->IsSpecObject()) return false;
- if (FLAG_harmony_concat_spreadable) {
- Handle<Symbol> key(isolate->factory()->is_concat_spreadable_symbol());
- Handle<Object> value;
- MaybeHandle<Object> maybeValue =
- i::Runtime::GetObjectProperty(isolate, obj, key);
- if (maybeValue.ToHandle(&value)) {
- if (!value->IsUndefined()) {
- return value->BooleanValue();
- }
- }
- }
- return obj->IsJSArray();
-}
-
-
/**
* Array::concat implementation.
* See ECMAScript 262, 15.4.4.4.
Index: test/mjsunit/harmony/array-concat.js
diff --git a/test/mjsunit/harmony/array-concat.js
b/test/mjsunit/harmony/array-concat.js
index
71b6790bc7b13e238930ed824d165d76ec903c2c..87bd246cbad7381d42f2a8c2ca294aa9023d0c61
100644
--- a/test/mjsunit/harmony/array-concat.js
+++ b/test/mjsunit/harmony/array-concat.js
@@ -706,3 +706,14 @@ function testConcatTypedArray(type, elems, modulo) {
assertEquals(1 + arr3.length * 2, r4.length);
assertEquals(expectedTrace, trace);
})();
+
+
+(function testConcatArrayNonSpreadable() {
+ "use strict";
+ var b = [];
+ b[Symbol.isConcatSpreadable] = false;
+ assertEquals(2, b.concat(b).length);
+ assertEquals(1, [].concat(b).length);
+ assertEquals(1, [].concat(b, new Array).length);
+ assertEquals(2, [].concat(b, new Object).length);
+})();
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
