Hi Yes was wrong in using IsNumber(), have corrected that, added tests and have submitted a CL here https://chromium-review.googlesource.com/c/v8/v8/+/4333698, I dont know who exactly to cc as reviewers hence posting here only.
Yours Sincerely, Debadree On Monday, March 13, 2023 at 4:41:37 PM UTC+5:30 Jakob Kummerow wrote: > On Mon, Mar 13, 2023 at 8:59 AM Debadree Chatterjee <[email protected]> > wrote: > >> Hi Leszek, >> >> Thank you so much for the response, will try to apply your suggestions >> and iron out any bugs and submit a CL, will ask here if encounter anything >> I don't understand >> Thank you once again! >> >> Yours Sincerely, >> Debadree >> >> On Monday, March 13, 2023 at 12:30:11 PM UTC+5:30 [email protected] >> wrote: >> >> Hi Debadree, >> >> Sorry for not replying to your original email - often with bugs like >> this, the reason we haven't fixed them is because no one thought about it >> too much, and thinking about the solution is 90% of the time needed to >> actually fix it. >> >> In your case, I haven't looked at the details of the issue whether your >> overall approach is correct - at a skim it seems right but you never know >> with weird edge cases. To answer your question about IsNumber, you likely >> want to also check for index strings, like "123". >> >> > I think that's "instead", not "also". Asking ->IsNumber() for something > that's typed Handle<Name> never makes sense, because it'll always return > false, because Names aren't Numbers. > > >> You can detect these with String::AsArrayIndex, which returns false if >> the conversion to a numeric index fails. >> >> Hope that's helpful, >> Leszek >> >> >> On Sat, 11 Mar 2023, 21:13 Debadree Chatterjee, <[email protected]> >> wrote: >> >> Anu guidance toward any relevant documentation would also be helpful >> >> On Thursday, March 2, 2023 at 12:16:36 AM UTC+5:30 Debadree Chatterjee >> wrote: >> >> Hello v8-dev, >> >> I am trying to work on the issue >> https://bugs.chromium.org/p/v8/issues/detail?id=13728 which is a bug >> noticed from nodejs, as a first time contributor the docs suggest that I >> reach out here first before working on a issue. Already some of the >> debugging of the issue has been done on the original thread on nodejs repo >> @ https://github.com/nodejs/node/issues/41714, the crux of the matter >> being that the function `FilterProxyKeys` seems to ignore the >> `v8::IndexFilter::kSkipIndices` property passed to `GetPropertyNames`, I >> attempted to solve this with the following diff >> >> ```diff >> diff --git a/deps/v8/src/objects/keys.cc b/deps/v8/src/objects/keys.cc >> index a0796864f1..4f84cd9094 100644 >> --- a/deps/v8/src/objects/keys.cc >> +++ b/deps/v8/src/objects/keys.cc >> @@ -182,8 +182,9 @@ ExceptionStatus >> KeyAccumulator::AddKeys(Handle<JSObject> array_like, >> MaybeHandle<FixedArray> FilterProxyKeys(KeyAccumulator* accumulator, >> Handle<JSProxy> owner, >> Handle<FixedArray> keys, >> - PropertyFilter filter) { >> - if (filter == ALL_PROPERTIES) { >> + PropertyFilter filter, >> + bool skip_indices) { >> + if (filter == ALL_PROPERTIES && !skip_indices) { >> // Nothing to do. >> return keys; >> } >> @@ -191,7 +192,7 @@ MaybeHandle<FixedArray> >> FilterProxyKeys(KeyAccumulator* accumulator, >> int store_position = 0; >> for (int i = 0; i < keys->length(); ++i) { >> Handle<Name> key(Name::cast(keys->get(i)), isolate); >> - if (key->FilterKey(filter)) continue; // Skip this key. >> + if (key->FilterKey(filter) || (skip_indices && key->IsNumber())) >> continue; // Skip this key. >> if (filter & ONLY_ENUMERABLE) { >> PropertyDescriptor desc; >> Maybe<bool> found = >> @@ -218,7 +219,7 @@ Maybe<bool> >> KeyAccumulator::AddKeysFromJSProxy(Handle<JSProxy> proxy, >> // Postpone the enumerable check for for-in to the ForInFilter step. >> if (!is_for_in_) { >> ASSIGN_RETURN_ON_EXCEPTION_VALUE( >> - isolate_, keys, FilterProxyKeys(this, proxy, keys, filter_), >> + isolate_, keys, FilterProxyKeys(this, proxy, keys, filter_, >> skip_indices_), >> Nothing<bool>()); >> } >> // >> https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys >> ``` >> >> but this doesn't seem to work mainly because I think I am using >> `key->IsNumber()` wrong. I am not sure what would be the correct way to >> check if the given key is indeed an index hence I ask for any guidance >> here, I understand this is a very beginner question but any help would be >> greatly appreciated. >> >> Your Sincerely, >> Debadree >> >> -- >> >> -- -- v8-dev mailing list [email protected] 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/508496c4-02d8-4fc7-97ca-b83486c849c5n%40googlegroups.com.
