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/CAKSzg3RmMzODE0pasZ8BBOLk8XwXqNeyQLoJ7t%2BY_gp0wF%3DS%3DA%40mail.gmail.com.
