Skip to content

Commit 74571c8

Browse files
BridgeARCommit Bot
BridgeAR
authored and
Commit Bot
committed
Fix preview of set entries
Set entries return an array with the value as first and second entry. As such these are considered key value pairs to align with maps entries iterator. So far the return value was identical to the values iterator and that is misleading. This also adds tests to verify the results and improves the coverage a tiny bit by testing different iterators. Refs: nodejs/node#24629 [email protected] Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253 Reviewed-on: https://chromium-review.googlesource.com/c/1350790 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#59311}
1 parent 58d5361 commit 74571c8

File tree

6 files changed

+278
-29
lines changed

6 files changed

+278
-29
lines changed

‎AUTHORS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ Rick Waldron <[email protected]>
148148
Rob Wu <[email protected]>
149149
Robert Mustacchi <[email protected]>
150150
Robert Nagy <[email protected]>
151+
Ruben Bridgewater <[email protected]>
151152
Ryan Dahl <[email protected]>
152153
Sakthipriyan Vairamani (thefourtheye) <[email protected]>
153154
Sander Mathijs van Veen <[email protected]>

‎src/api.cc‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6985,6 +6985,11 @@ enum class MapAsArrayKind {
69856985
kValues = i::JS_MAP_VALUE_ITERATOR_TYPE
69866986
};
69876987

6988+
enum class SetAsArrayKind {
6989+
kEntries = i::JS_SET_KEY_VALUE_ITERATOR_TYPE,
6990+
kValues = i::JS_SET_VALUE_ITERATOR_TYPE
6991+
};
6992+
69886993
i::Handle<i::JSArray> MapAsArray(i::Isolate* isolate, i::Object table_obj,
69896994
int offset, MapAsArrayKind kind) {
69906995
i::Factory* factory = isolate->factory();
@@ -7094,13 +7099,14 @@ Maybe<bool> Set::Delete(Local<Context> context, Local<Value> key) {
70947099

70957100
namespace {
70967101
i::Handle<i::JSArray> SetAsArray(i::Isolate* isolate, i::Object table_obj,
7097-
int offset) {
7102+
int offset, SetAsArrayKind kind) {
70987103
i::Factory* factory = isolate->factory();
70997104
i::Handle<i::OrderedHashSet> table(i::OrderedHashSet::cast(table_obj),
71007105
isolate);
71017106
// Elements skipped by |offset| may already be deleted.
71027107
int capacity = table->UsedCapacity();
7103-
int max_length = capacity - offset;
7108+
const bool collect_key_values = kind == SetAsArrayKind::kEntries;
7109+
int max_length = (capacity - offset) * (collect_key_values ? 2 : 1);
71047110
if (max_length == 0) return factory->NewJSArray(0);
71057111
i::Handle<i::FixedArray> result = factory->NewFixedArray(max_length);
71067112
int result_index = 0;
@@ -7111,6 +7117,7 @@ i::Handle<i::JSArray> SetAsArray(i::Isolate* isolate, i::Object table_obj,
71117117
i::Object key = table->KeyAt(i);
71127118
if (key == the_hole) continue;
71137119
result->set(result_index++, key);
7120+
if (collect_key_values) result->set(result_index++, key);
71147121
}
71157122
}
71167123
DCHECK_GE(max_length, result_index);
@@ -7126,7 +7133,8 @@ Local<Array> Set::AsArray() const {
71267133
i::Isolate* isolate = obj->GetIsolate();
71277134
LOG_API(isolate, Set, AsArray);
71287135
ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
7129-
return Utils::ToLocal(SetAsArray(isolate, obj->table(), 0));
7136+
return Utils::ToLocal(
7137+
SetAsArray(isolate, obj->table(), 0, SetAsArrayKind::kValues));
71307138
}
71317139

71327140

@@ -9553,21 +9561,22 @@ v8::MaybeLocal<v8::Array> v8::Object::PreviewEntries(bool* is_key_value) {
95539561
i::Handle<i::JSWeakCollection>::cast(object), 0));
95549562
}
95559563
if (object->IsJSMapIterator()) {
9556-
i::Handle<i::JSMapIterator> iterator =
9557-
i::Handle<i::JSMapIterator>::cast(object);
9564+
i::Handle<i::JSMapIterator> it = i::Handle<i::JSMapIterator>::cast(object);
95589565
MapAsArrayKind const kind =
9559-
static_cast<MapAsArrayKind>(iterator->map()->instance_type());
9566+
static_cast<MapAsArrayKind>(it->map()->instance_type());
95609567
*is_key_value = kind == MapAsArrayKind::kEntries;
9561-
if (!iterator->HasMore()) return v8::Array::New(v8_isolate);
9562-
return Utils::ToLocal(MapAsArray(isolate, iterator->table(),
9563-
i::Smi::ToInt(iterator->index()), kind));
9568+
if (!it->HasMore()) return v8::Array::New(v8_isolate);
9569+
return Utils::ToLocal(
9570+
MapAsArray(isolate, it->table(), i::Smi::ToInt(it->index()), kind));
95649571
}
95659572
if (object->IsJSSetIterator()) {
95669573
i::Handle<i::JSSetIterator> it = i::Handle<i::JSSetIterator>::cast(object);
9567-
*is_key_value = false;
9574+
SetAsArrayKind const kind =
9575+
static_cast<SetAsArrayKind>(it->map()->instance_type());
9576+
*is_key_value = kind == SetAsArrayKind::kEntries;
95689577
if (!it->HasMore()) return v8::Array::New(v8_isolate);
95699578
return Utils::ToLocal(
9570-
SetAsArray(isolate, it->table(), i::Smi::ToInt(it->index())));
9579+
SetAsArray(isolate, it->table(), i::Smi::ToInt(it->index()), kind));
95719580
}
95729581
return v8::MaybeLocal<v8::Array>();
95739582
}

‎test/cctest/test-api.cc‎

Lines changed: 228 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28649,7 +28649,7 @@ TEST(MicrotaskContextShouldBeNativeContext) {
2864928649
isolate->RunMicrotasks();
2865028650
}
2865128651

28652-
TEST(PreviewSetIteratorEntriesWithDeleted) {
28652+
TEST(PreviewSetKeysIteratorEntriesWithDeleted) {
2865328653
LocalContext env;
2865428654
v8::HandleScope handle_scope(env->GetIsolate());
2865528655
v8::Local<v8::Context> context = env.local();
@@ -28748,7 +28748,142 @@ TEST(PreviewSetIteratorEntriesWithDeleted) {
2874828748
}
2874928749
}
2875028750

28751-
TEST(PreviewMapIteratorEntriesWithDeleted) {
28751+
TEST(PreviewSetValuesIteratorEntriesWithDeleted) {
28752+
LocalContext env;
28753+
v8::HandleScope handle_scope(env->GetIsolate());
28754+
v8::Local<v8::Context> context = env.local();
28755+
28756+
{
28757+
// Create set, delete entry, create iterator, preview.
28758+
v8::Local<v8::Object> iterator =
28759+
CompileRun("var set = new Set([1,2,3]); set.delete(1); set.values()")
28760+
->ToObject(context)
28761+
.ToLocalChecked();
28762+
bool is_key;
28763+
v8::Local<v8::Array> entries =
28764+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28765+
CHECK(!is_key);
28766+
CHECK_EQ(2, entries->Length());
28767+
CHECK_EQ(2, entries->Get(context, 0)
28768+
.ToLocalChecked()
28769+
->Int32Value(context)
28770+
.FromJust());
28771+
CHECK_EQ(3, entries->Get(context, 1)
28772+
.ToLocalChecked()
28773+
->Int32Value(context)
28774+
.FromJust());
28775+
}
28776+
{
28777+
// Create set, create iterator, delete entry, preview.
28778+
v8::Local<v8::Object> iterator =
28779+
CompileRun("var set = new Set([1,2,3]); set.values()")
28780+
->ToObject(context)
28781+
.ToLocalChecked();
28782+
CompileRun("set.delete(1);");
28783+
bool is_key;
28784+
v8::Local<v8::Array> entries =
28785+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28786+
CHECK(!is_key);
28787+
CHECK_EQ(2, entries->Length());
28788+
CHECK_EQ(2, entries->Get(context, 0)
28789+
.ToLocalChecked()
28790+
->Int32Value(context)
28791+
.FromJust());
28792+
CHECK_EQ(3, entries->Get(context, 1)
28793+
.ToLocalChecked()
28794+
->Int32Value(context)
28795+
.FromJust());
28796+
}
28797+
{
28798+
// Create set, create iterator, delete entry, iterate, preview.
28799+
v8::Local<v8::Object> iterator =
28800+
CompileRun("var set = new Set([1,2,3]); var it = set.values(); it")
28801+
->ToObject(context)
28802+
.ToLocalChecked();
28803+
CompileRun("set.delete(1); it.next();");
28804+
bool is_key;
28805+
v8::Local<v8::Array> entries =
28806+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28807+
CHECK(!is_key);
28808+
CHECK_EQ(1, entries->Length());
28809+
CHECK_EQ(3, entries->Get(context, 0)
28810+
.ToLocalChecked()
28811+
->Int32Value(context)
28812+
.FromJust());
28813+
}
28814+
{
28815+
// Create set, create iterator, delete entry, iterate until empty, preview.
28816+
v8::Local<v8::Object> iterator =
28817+
CompileRun("var set = new Set([1,2,3]); var it = set.values(); it")
28818+
->ToObject(context)
28819+
.ToLocalChecked();
28820+
CompileRun("set.delete(1); it.next(); it.next();");
28821+
bool is_key;
28822+
v8::Local<v8::Array> entries =
28823+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28824+
CHECK(!is_key);
28825+
CHECK_EQ(0, entries->Length());
28826+
}
28827+
{
28828+
// Create set, create iterator, delete entry, iterate, trigger rehash,
28829+
// preview.
28830+
v8::Local<v8::Object> iterator =
28831+
CompileRun("var set = new Set([1,2,3]); var it = set.values(); it")
28832+
->ToObject(context)
28833+
.ToLocalChecked();
28834+
CompileRun("set.delete(1); it.next();");
28835+
CompileRun("for (var i = 4; i < 20; i++) set.add(i);");
28836+
bool is_key;
28837+
v8::Local<v8::Array> entries =
28838+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28839+
CHECK(!is_key);
28840+
CHECK_EQ(17, entries->Length());
28841+
for (uint32_t i = 0; i < 17; i++) {
28842+
CHECK_EQ(i + 3, entries->Get(context, i)
28843+
.ToLocalChecked()
28844+
->Int32Value(context)
28845+
.FromJust());
28846+
}
28847+
}
28848+
}
28849+
28850+
TEST(PreviewMapEntriesIteratorEntries) {
28851+
LocalContext env;
28852+
v8::HandleScope handle_scope(env->GetIsolate());
28853+
v8::Local<v8::Context> context = env.local();
28854+
{
28855+
// Create set, delete entry, create entries iterator, preview.
28856+
v8::Local<v8::Object> iterator =
28857+
CompileRun("var set = new Set([1,2,3]); set.delete(2); set.entries()")
28858+
->ToObject(context)
28859+
.ToLocalChecked();
28860+
bool is_key;
28861+
v8::Local<v8::Array> entries =
28862+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28863+
CHECK(is_key);
28864+
CHECK_EQ(4, entries->Length());
28865+
uint32_t first = entries->Get(context, 0)
28866+
.ToLocalChecked()
28867+
->Int32Value(context)
28868+
.FromJust();
28869+
uint32_t second = entries->Get(context, 2)
28870+
.ToLocalChecked()
28871+
->Int32Value(context)
28872+
.FromJust();
28873+
CHECK_EQ(1, first);
28874+
CHECK_EQ(3, second);
28875+
CHECK_EQ(first, entries->Get(context, 1)
28876+
.ToLocalChecked()
28877+
->Int32Value(context)
28878+
.FromJust());
28879+
CHECK_EQ(second, entries->Get(context, 3)
28880+
.ToLocalChecked()
28881+
->Int32Value(context)
28882+
.FromJust());
28883+
}
28884+
}
28885+
28886+
TEST(PreviewMapValuesIteratorEntriesWithDeleted) {
2875228887
LocalContext env;
2875328888
v8::HandleScope handle_scope(env->GetIsolate());
2875428889
v8::Local<v8::Context> context = env.local();
@@ -28863,6 +28998,97 @@ TEST(PreviewMapIteratorEntriesWithDeleted) {
2886328998
}
2886428999
}
2886529000

29001+
TEST(PreviewMapKeysIteratorEntriesWithDeleted) {
29002+
LocalContext env;
29003+
v8::HandleScope handle_scope(env->GetIsolate());
29004+
v8::Local<v8::Context> context = env.local();
29005+
29006+
{
29007+
// Create map, delete entry, create iterator, preview.
29008+
v8::Local<v8::Object> iterator = CompileRun(
29009+
"var map = new Map();"
29010+
"var key = 1; map.set(key, {});"
29011+
"map.set(2, {}); map.set(3, {});"
29012+
"map.delete(key);"
29013+
"map.keys()")
29014+
->ToObject(context)
29015+
.ToLocalChecked();
29016+
bool is_key;
29017+
v8::Local<v8::Array> entries =
29018+
iterator->PreviewEntries(&is_key).ToLocalChecked();
29019+
CHECK(!is_key);
29020+
CHECK_EQ(2, entries->Length());
29021+
CHECK_EQ(2, entries->Get(context, 0)
29022+
.ToLocalChecked()
29023+
->Int32Value(context)
29024+
.FromJust());
29025+
CHECK_EQ(3, entries->Get(context, 1)
29026+
.ToLocalChecked()
29027+
->Int32Value(context)
29028+
.FromJust());
29029+
}
29030+
{
29031+
// Create map, create iterator, delete entry, preview.
29032+
v8::Local<v8::Object> iterator = CompileRun(
29033+
"var map = new Map();"
29034+
"var key = 1; map.set(key, {});"
29035+
"map.set(2, {}); map.set(3, {});"
29036+
"map.keys()")
29037+
->ToObject(context)
29038+
.ToLocalChecked();
29039+
CompileRun("map.delete(key);");
29040+
bool is_key;
29041+
v8::Local<v8::Array> entries =
29042+
iterator->PreviewEntries(&is_key).ToLocalChecked();
29043+
CHECK(!is_key);
29044+
CHECK_EQ(2, entries->Length());
29045+
CHECK_EQ(2, entries->Get(context, 0)
29046+
.ToLocalChecked()
29047+
->Int32Value(context)
29048+
.FromJust());
29049+
CHECK_EQ(3, entries->Get(context, 1)
29050+
.ToLocalChecked()
29051+
->Int32Value(context)
29052+
.FromJust());
29053+
}
29054+
{
29055+
// Create map, create iterator, delete entry, iterate, preview.
29056+
v8::Local<v8::Object> iterator = CompileRun(
29057+
"var map = new Map();"
29058+
"var key = 1; map.set(key, {});"
29059+
"map.set(2, {}); map.set(3, {});"
29060+
"var it = map.keys(); it")
29061+
->ToObject(context)
29062+
.ToLocalChecked();
29063+
CompileRun("map.delete(key); it.next();");
29064+
bool is_key;
29065+
v8::Local<v8::Array> entries =
29066+
iterator->PreviewEntries(&is_key).ToLocalChecked();
29067+
CHECK(!is_key);
29068+
CHECK_EQ(1, entries->Length());
29069+
CHECK_EQ(3, entries->Get(context, 0)
29070+
.ToLocalChecked()
29071+
->Int32Value(context)
29072+
.FromJust());
29073+
}
29074+
{
29075+
// Create map, create iterator, delete entry, iterate until empty, preview.
29076+
v8::Local<v8::Object> iterator = CompileRun(
29077+
"var map = new Map();"
29078+
"var key = 1; map.set(key, {});"
29079+
"map.set(2, {}); map.set(3, {});"
29080+
"var it = map.keys(); it")
29081+
->ToObject(context)
29082+
.ToLocalChecked();
29083+
CompileRun("map.delete(key); it.next(); it.next();");
29084+
bool is_key;
29085+
v8::Local<v8::Array> entries =
29086+
iterator->PreviewEntries(&is_key).ToLocalChecked();
29087+
CHECK(!is_key);
29088+
CHECK_EQ(0, entries->Length());
29089+
}
29090+
}
29091+
2886629092
namespace {
2886729093
static v8::Isolate* isolate_1;
2886829094
static v8::Isolate* isolate_2;

‎test/inspector/debugger/object-preview-internal-properties-expected.txt‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -166,27 +166,39 @@ expression: (new Map([[1,2]])).entries()
166166
}
167167
]
168168

169-
expression: (new Set([[1,2]])).entries()
169+
expression: (new Set([1,2])).entries()
170170
[[Entries]]:
171171
[
172172
[0] : {
173+
key : {
174+
description : 1
175+
overflow : false
176+
properties : [
177+
]
178+
type : number
179+
}
173180
value : {
174-
description : Array(2)
181+
description : 1
175182
overflow : false
176183
properties : [
177-
[0] : {
178-
name : 0
179-
type : number
180-
value : 1
181-
}
182-
[1] : {
183-
name : 1
184-
type : number
185-
value : 2
186-
}
187184
]
188-
subtype : array
189-
type : object
185+
type : number
186+
}
187+
}
188+
[1] : {
189+
key : {
190+
description : 2
191+
overflow : false
192+
properties : [
193+
]
194+
type : number
195+
}
196+
value : {
197+
description : 2
198+
overflow : false
199+
properties : [
200+
]
201+
type : number
190202
}
191203
}
192204
]

‎test/inspector/debugger/object-preview-internal-properties.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ InspectorTest.runTestSuite([
4646
function iteratorObject(next)
4747
{
4848
checkExpression("(new Map([[1,2]])).entries()")
49-
.then(() => checkExpression("(new Set([[1,2]])).entries()"))
49+
.then(() => checkExpression("(new Set([1,2])).entries()"))
5050
.then(next);
5151
},
5252

0 commit comments

Comments
 (0)