On Tue, Feb 26, 2013 at 8:36 AM, <[email protected]> wrote: > On 2013/02/26 16:19:30, adamk wrote: > >> On 2013/02/26 12:01:45, rossberg wrote: >> > On 2013/02/25 19:36:03, adamk wrote: >> > > https://codereview.chromium.**org/12328064/diff/1/src/**objects.cc<https://codereview.chromium.org/12328064/diff/1/src/objects.cc> >> > > File src/objects.cc (left): >> > > >> > > https://codereview.chromium.**org/12328064/diff/1/src/** >> objects.cc#oldcode3373<https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373> >> > > src/objects.cc:3373: // Handle [] on String objects. >> > > On 2013/02/25 10:46:25, rossberg wrote: >> > > > I'm not sure removing this is correct. This function is also called >> by >> > > > GetPropertyAttributeWithReceiv**er, and on that path, I don't think >> > strings > >> > > would >> > > > be handled otherwise. >> > > >> > > I've added a test case covering this case, and it still passes. The >> reason >> is >> > > that, although I've removed the check here, the very next thing this >> code >> does >> > > is call GetElementAttributeWithoutInte**rceptor, which itself does >> the >> > > IsStringObjectWithCharacterAt check. Please take a look at the test >> case >> > and > >> > see >> > > if there's other stuff that I need to cover. >> > >> > Hm, I'm not sure what the simplest JS test is then, but I'm pretty >> certain >> that >> > the following API code will fail after the change: >> > >> > Handle<String> s = String::New("JS rocks so hard"); >> > ASSERT(s->Has(1)); >> > > I assume you mean StringObject here, not String? Anyway, that works fine >> (I >> > can > >> upload a test case when I get in; looks like StringObject doesn't have >> any API >> coverage). >> > > Yes, sorry, I managed to forget that wrapping has to be done manually > outside JS > land :). So more like: > > > Handle<String> s = String::New("JS rocks so hard"); > Handle<StringObject> o = StringObject::New(s); > ASSERT(o->Has(1)); > > > Perhaps there's some confusion here? The only way the code I'm removing >> could >> have an impact is if, somehow, you managed to have a JSValue representing >> a >> String that _also_ managed to have elements for the same indices as the >> String >> (in the same Object, not in a prototype), and those elements had different >> attributes. But I don't think this is possible, since it's not possible >> to add >> elements to a JSValue _before_ creation, and after creation those indices >> are >> read-only. >> > > I'm not sure I follow. AFAICS, JSReceiver::HasElement calls > JSObject::**GetElementAttributeWithReceive**r, which is where you removed > the > special case for string objects. And the call to Has in the example above > should > take that path, shouldn't it? If not, I'm indeed confused, where on earth > is it > going? >
The call sequence is: JSReceiver::HasElement JSObject::GetElementAttributeWithReceiver JSObject::GetElementAttributeWithoutInterceptor GetElementAttributeWithoutInterceptor() does two things: 1. Check if the index is present in the element backing store 2. If not, check if this is a string object with a character at that index The call sequence for "'0' in o" is: JSReceiver::HasProperty JSObject::GetPropertyAttribute JSObject::GetPropertyAttributeWithReceiver JSObject::GetElementAttributeWithReceiver JSObject::GetElementAttributeWithoutInterceptor I think I shot myself in the foot here by removing the HandleScope from GetElementAttributeWithoutInterceptor in the same change, which obscures the fact that its call to IsStringObjectWithCharacterAt remains unchanged. - Adam -- -- 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]. For more options, visit https://groups.google.com/groups/opt_out.
