Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Rolf Eike Beer]

> On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 433
> > 
> >
> > more * inconsistency
> 
> Bernd Buschinski wrote:
> I don't get the problem here, could you please explain?

GetterSetterImp *gs -> GetterSetterImp* gs


- Rolf Eike


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review13085
---


On June 1, 2012, 12:34 p.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> ---
> 
> (Updated June 1, 2012, 12:34 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
> 
> This is a pretty big patch, to get Object.defineProperty perfect for 
> ecmascript (for all tests that only use implemented stuff, all test that use 
> Object.create for example will fail, as its not implemented)
> 
> PropertyDescriptor:
> Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
> might requiere to rebuild khtml against new kjs, otherwise it might cause 
> weird crashes (at least for me)
> 
> 
> object.h:
> Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
> defineOwnProperty, the important changes are making
> getPropertyAttributes, put/get/remove-Direct virtual.
> Why do I need that?
> Because put checks if the prototype already has property XYZ and uses it. Now 
> imagine an array that got a setter-only property via a prototype. 
> DefineProperty would try to use put, which uses the prototype property and it 
> would fail. So all custom-data classes like Array need to implement/use 
> put/get/remove-Direct.
> 
> 
> object.cpp:
> currently put on a setter-only property would always throw an exception, this 
> is only correct for strict-mode, as we currently do not check for strict-mode 
> it would make more sense to change it to default not throwing an exception.
> 
> 
> array.cpp/h:
> The old Array implementation did not store attributes for array indexes, I 
> rewrote it to also store the attributes.
> + Bonus: also fix getOwnPropertyNames, as we now store attributes.
> + use new attributes, reject put/delete/enum if set
> 
> function.cpp (Arguments)
> changed the default attributes how parameter are stored, according to ECMA 
> 10.6.11.b
> 
> 
> Rest is "just" the defineProperty implementation
> 
> 
> Diffs
> -
> 
>   kjs/CMakeLists.txt 1188064 
>   kjs/CommonIdentifiers.h 8ee40e8 
>   kjs/array_instance.h 3f2b630 
>   kjs/array_instance.cpp fe9b8b4 
>   kjs/function.h 3757fe8 
>   kjs/function.cpp 5f39ae6 
>   kjs/object.h 047c242 
>   kjs/object.cpp c19122f 
>   kjs/object_object.cpp 986f03f 
>   kjs/operations.h f8a28c8 
>   kjs/operations.cpp d4c0066 
>   kjs/propertydescriptor.h PRE-CREATION 
>   kjs/propertydescriptor.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/104515/diff/
> 
> 
> Testing
> ---
> 
> ecmascript & daily surfing
> 
> used valgrind on many array testcases to check for possible memleaks
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated June 1, 2012, 12:34 p.m.)


Review request for kdelibs.


Changes
---

And here have the correct version of the patch.

The last version lacked the updated defineOwnProperty for arguments


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 
  kjs/propertydescriptor.h PRE-CREATION 
  kjs/propertydescriptor.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated May 31, 2012, 8:46 p.m.)


Review request for kdelibs.


Changes
---

Updated "arguments" handling,
as we use putDirect in all places we also need it for the arguments,
same for getDirect.

Also fixed a bug in deleting arguments indexes.


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 
  kjs/propertydescriptor.h PRE-CREATION 
  kjs/propertydescriptor.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated May 2, 2012, 8:23 p.m.)


Review request for kdelibs.


Changes
---

array_instance.cpp
more the length check for index into putDirect, as I wrongly used one put 
Object::defineOwnProperty that should be putDirect (last change)

object.cpp
getOwnPropertyDescriptor, using only getDirect will fail for prototypes, as 
they don't implement getDirect to get propertys,
we have to check the getOwnPropertySlot and use that to get the value.


"more * inconsistency"
https://git.reviewboard.kde.org/r/104515/diff/4/?file=58277#file58277line433

which I don't understand, is still open


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 
  kjs/propertydescriptor.h PRE-CREATION 
  kjs/propertydescriptor.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated April 30, 2012, 7:11 p.m.)


Review request for kdelibs.


Changes
---

Fixed all issues expect for the

"more * inconsistency"
https://git.reviewboard.kde.org/r/104515/diff/4/?file=58277#file58277line433

which I don't understand


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 
  kjs/propertydescriptor.h PRE-CREATION 
  kjs/propertydescriptor.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

> On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 433
> > 
> >
> > more * inconsistency

I don't get the problem here, could you please explain?


> On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 481
> > 
> >
> > *

same here, no clue whats wrong


> On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
> > kjs/object_object.cpp, line 288
> > 
> >
> > This can throw, I think. Yes, we probably have zillions of bugs where 
> > we don't check ->hadException precisely. On that note, I would discourage 
> > using toString in exception error messages, for the same reason --- you can 
> > get an error produced just trying to construct them. Instead, you'll 
> > probably want to factor out the code in printInfo() in internal.cpp into 
> > something that can make you a string, and use that.
> >

Hm yes, you are right, but I would like to fix this issue later in another 
patch.


> On April 29, 2012, 6:26 p.m., Maks Orlovich wrote:
> > kjs/propertydescriptor.h, line 36
> > 
> >
> > Are you sure you want them copyable? At any rate, you should never have 
> > a copy constructor but no operator=

ok, copy-ctor removed instead I will rely on the implicit operator=, which does 
the right thing because all objects are POD in this case.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review13085
---


On April 20, 2012, 11:55 a.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> ---
> 
> (Updated April 20, 2012, 11:55 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
> 
> This is a pretty big patch, to get Object.defineProperty perfect for 
> ecmascript (for all tests that only use implemented stuff, all test that use 
> Object.create for example will fail, as its not implemented)
> 
> PropertyDescriptor:
> Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
> might requiere to rebuild khtml against new kjs, otherwise it might cause 
> weird crashes (at least for me)
> 
> 
> object.h:
> Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
> defineOwnProperty, the important changes are making
> getPropertyAttributes, put/get/remove-Direct virtual.
> Why do I need that?
> Because put checks if the prototype already has property XYZ and uses it. Now 
> imagine an array that got a setter-only property via a prototype. 
> DefineProperty would try to use put, which uses the prototype property and it 
> would fail. So all custom-data classes like Array need to implement/use 
> put/get/remove-Direct.
> 
> 
> object.cpp:
> currently put on a setter-only property would always throw an exception, this 
> is only correct for strict-mode, as we currently do not check for strict-mode 
> it would make more sense to change it to default not throwing an exception.
> 
> 
> array.cpp/h:
> The old Array implementation did not store attributes for array indexes, I 
> rewrote it to also store the attributes.
> + Bonus: also fix getOwnPropertyNames, as we now store attributes.
> + use new attributes, reject put/delete/enum if set
> 
> function.cpp (Arguments)
> changed the default attributes how parameter are stored, according to ECMA 
> 10.6.11.b
> 
> 
> Rest is "just" the defineProperty implementation
> 
> 
> Diffs
> -
> 
>   kjs/CMakeLists.txt 1188064 
>   kjs/CommonIdentifiers.h 8ee40e8 
>   kjs/array_instance.h 3f2b630 
>   kjs/array_instance.cpp fe9b8b4 
>   kjs/function.h 3757fe8 
>   kjs/function.cpp 5f39ae6 
>   kjs/object.h 047c242 
>   kjs/object.cpp c19122f 
>   kjs/object_object.cpp 986f03f 
>   kjs/operations.h f8a28c8 
>   kjs/operations.cpp d4c0066 
>   kjs/propertydescriptor.h PRE-CREATION 
>   kjs/propertydescriptor.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/104515/diff/
> 
> 
> Testing
> ---
> 
> ecmascript & daily surfing
> 
> used valgrind on many array testcases to check for possible memleaks
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Maks Orlovich]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review13085
---


Still need more big picture stuff...


kjs/object.h


The comment "only looks at the property map" is no longer true; and in 
general, if you're going to make so much of stuff virtual, this needs heavy 
comments on what gets called where. When do I need to implement 
getOwnPropertySlot, when do I need getDirect?  getOwnPropertyDescriptor? 
getPropertyAttributes?

Ditto for the put family.





kjs/object.h


Should this particular override really be virtual?



kjs/object.cpp


*



kjs/object.cpp


This is rather hard to follow; please try consider giving higher-level 
description of what the cases do.



kjs/object.cpp


more * inconsistency



kjs/object.cpp


When is this ever not true when you're in this branch?



kjs/object.cpp


*



kjs/object.cpp


Not putDirect?



kjs/object_object.cpp


This can throw, I think. Yes, we probably have zillions of bugs where we 
don't check ->hadException precisely. On that note, I would discourage using 
toString in exception error messages, for the same reason --- you can get an 
error produced just trying to construct them. Instead, you'll probably want to 
factor out the code in printInfo() in internal.cpp into something that can make 
you a string, and use that.




kjs/operations.cpp


What I meant wrt to this function that it'd be helpful to put in how it's 
different from strictEquals in a comment, and why both exist (e.g. operator === 
vs. descriptor-type stuff)




kjs/propertydescriptor.h


Are you sure you want them copyable? At any rate, you should never have a 
copy constructor but no operator=



kjs/propertydescriptor.h


Hmm, I find these names a bit weird on PropertyDescriptor methods; it feels 
more like it should be setFromObject or such instead. (But feel free to ignore 
this suggestion, as it seems like it'd be a pain to change)



kjs/propertydescriptor.h


One thing we don't need is even more inconsistency in * placement..

More importantly, looking at the code this has very different behavior from 
the above. Please give it a different name and document the difference. 



kjs/propertydescriptor.h


What do these mean?



kjs/propertydescriptor.h


What does this do?



kjs/propertydescriptor.h


Feels weird to have both an equalTo and a private(!) equality operator.



kjs/propertydescriptor.cpp


Doesn't make sense in a function taking a JSObject*. Either make it take a 
JSValue* (which will simplify callers!) or remove.



kjs/propertydescriptor.cpp


Looks redundant.



kjs/propertydescriptor.cpp


This may be worth splitting up; it's certainly beyond my ability to read. 
At very least, you could have a helper for the 

foo == bar || (foo && bar && sameValue(exec, m_value, other.value()) 
pattern, too.




kjs/propertydescriptor.cpp


I think parentheses would be helpful here.



kjs/propertydescriptor.cpp


Unused, I think (and different semantics from equalTo)?


- Maks Orlovich


On April 20, 2012, 11:55 a.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> ---
> 
> (Updated April 20, 2012, 11:55 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
> 
> This is a pretty big patch, to get Object.defineProperty perfect for 
> ecmascript (for all tests

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated April 20, 2012, 11:55 a.m.)


Review request for kdelibs.


Changes
---

Fix ArrayInstance::defineOwnProperty (ECMAScript Edition 5.1r6 - 15.4.5.1) Step 
3d, return after Exception

While ECMAScript explicitly tells to return in all other situations, it does 
not in this case.
But it only makes sense to not apply the invalid new length, otherwise we can 
define any invalid array length.
+ Some tests show that we must not continue


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 
  kjs/propertydescriptor.h PRE-CREATION 
  kjs/propertydescriptor.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated April 12, 2012, 11:01 p.m.)


Review request for kdelibs.


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 
  kjs/propertydescriptor.h PRE-CREATION 
  kjs/propertydescriptor.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/CMakeLists.txt, line 193
> > 
> >
> > where is this file?
> >

good question, sorry, I will include it in the next update


> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/array_instance.cpp, line 55
> > 
> >
> > This is useless; it does what the default one would do (and why not 
> > copy constructor then?)

ok, dropped


> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 168
> > 
> >
> > I don't understand all that you're doing here --- you invoke 
> > getOwnPropertySlot (which is virtual and may be overridden in subclasses, 
> > then getPropertyAttributes and then getDirect. And then get. What's the 
> > interface to the subclass here? Just how many look ups is it doing, etc.?
> >

uhm yes, thats a left over from trying to fight without virtual getDirect, will 
reduce it to getDirect and getPropertyAttributes.


> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/object.cpp, line 444
> > 
> >
> > There is an explicit isUndefined check, but what about the ways in 
> > which toObject can fail?

in propertydescriptor is checked, or rather it is checked via implementsCall.
If it does not, we will not even got to this point.


> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/operations.cpp, line 196
> > 
> >
> > Please point out how this isn't the same as strictEquals (due to the 
> > spec having two almost identical, but slightly different in handling of FP 
> > ops).
> >

Yes, its only different in the number check. The +0 != 0, and both NaN check.
in C++ NAN == NAN -> false (which is strictEquals)
for sameValue NAN == NAN -> true

same for +0 and 0. There are some testcases which rely on this


> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/array_instance.cpp, line 42
> > 
> >
> > Could the array stuff perhaps be split out?

Splitted out? to where? I would like to keep the array implementation in the 
.cpp 
so that it can be changed without any ABI/API changes :)


> On April 10, 2012, 2:54 a.m., Maks Orlovich wrote:
> > kjs/object.h, line 458
> > 
> >
> > This is why I don't like the making of these virtual: getDirectLocation 
> > goes with these, so if these are virtual so should it be, but 
> > getDirectLocation is used by the Window object, so it's absolutely and 
> > utterly performance-critical.
> > 
> > What I don't understand is: is there actually generic/polymorphic code 
> > that needs to call both the default and array's version?
> >

First of, Array::defineOwnProperty calls Object::defineOwnProperty for every 
array index, Array::defineOwnProperty does not store any data for array indexes.

And the last step of Object::defineOwnProperty is like everything is allowed.
Most Important is it manipulates the getter/setter directly.

And if Array::defineOwnProperty calls Object::defineOwnProperty, now (in the 
last step) we need to direct get it.
This might be an array index, so getDirect is needed.
After we changed it, we need to store it directly (with its new attributes), 
putDirect is needed.

Also without virtual putDirect we can't store anything correctly if the 
prototype has this property as getter/setter


In general putDirect is needed for every getter/setter put. As we can not 
update getter/setter using put.
And we also need to update the attributes, put checks that, but we just want to 
put them without checks in Object::defineOwnProperty


Back to getDirectLocation, luckily I don't need it. (guess that doesn't satisfy 
you)


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review12282
---


On April 9, 2012, 8:23 p.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> ---
> 
> (Updated April 9, 2012, 8:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
> 
> This is a pretty big patch, to get Object.defineProperty perfect for 
> ecmascript (for all tests that only use implemented stuff, all test that use 
> Object.cre

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Maks Orlovich]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/#review12282
---


I think I need a bit of higher-level info from here; plus I guess refining the 
other one (which has far simpler stuff to fix) and submitting it would remove 
some of the duplication..


kjs/CMakeLists.txt


where is this file?




kjs/array_instance.cpp


Could the array stuff perhaps be split out?



kjs/array_instance.cpp


This is useless; it does what the default one would do (and why not copy 
constructor then?)



kjs/object.h


This is why I don't like the making of these virtual: getDirectLocation 
goes with these, so if these are virtual so should it be, but getDirectLocation 
is used by the Window object, so it's absolutely and utterly 
performance-critical.

What I don't understand is: is there actually generic/polymorphic code that 
needs to call both the default and array's version?




kjs/object.cpp


I don't understand all that you're doing here --- you invoke 
getOwnPropertySlot (which is virtual and may be overridden in subclasses, then 
getPropertyAttributes and then getDirect. And then get. What's the interface to 
the subclass here? Just how many look ups is it doing, etc.?




kjs/object.cpp


There is an explicit isUndefined check, but what about the ways in which 
toObject can fail?



kjs/operations.cpp


Please point out how this isn't the same as strictEquals (due to the spec 
having two almost identical, but slightly different in handling of FP ops).



- Maks Orlovich


On April 9, 2012, 8:23 p.m., Bernd Buschinski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104515/
> ---
> 
> (Updated April 9, 2012, 8:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty
> 
> This is a pretty big patch, to get Object.defineProperty perfect for 
> ecmascript (for all tests that only use implemented stuff, all test that use 
> Object.create for example will fail, as its not implemented)
> 
> PropertyDescriptor:
> Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
> might requiere to rebuild khtml against new kjs, otherwise it might cause 
> weird crashes (at least for me)
> 
> 
> object.h:
> Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
> defineOwnProperty, the important changes are making
> getPropertyAttributes, put/get/remove-Direct virtual.
> Why do I need that?
> Because put checks if the prototype already has property XYZ and uses it. Now 
> imagine an array that got a setter-only property via a prototype. 
> DefineProperty would try to use put, which uses the prototype property and it 
> would fail. So all custom-data classes like Array need to implement/use 
> put/get/remove-Direct.
> 
> 
> object.cpp:
> currently put on a setter-only property would always throw an exception, this 
> is only correct for strict-mode, as we currently do not check for strict-mode 
> it would make more sense to change it to default not throwing an exception.
> 
> 
> array.cpp/h:
> The old Array implementation did not store attributes for array indexes, I 
> rewrote it to also store the attributes.
> + Bonus: also fix getOwnPropertyNames, as we now store attributes.
> + use new attributes, reject put/delete/enum if set
> 
> function.cpp (Arguments)
> changed the default attributes how parameter are stored, according to ECMA 
> 10.6.11.b
> 
> 
> Rest is "just" the defineProperty implementation
> 
> 
> Diffs
> -
> 
>   kjs/CMakeLists.txt 1188064 
>   kjs/CommonIdentifiers.h 8ee40e8 
>   kjs/array_instance.h 3f2b630 
>   kjs/array_instance.cpp fe9b8b4 
>   kjs/function.h 3757fe8 
>   kjs/function.cpp 5f39ae6 
>   kjs/object.h 047c242 
>   kjs/object.cpp c19122f 
>   kjs/object_object.cpp 986f03f 
>   kjs/operations.h f8a28c8 
>   kjs/operations.cpp d4c0066 
> 
> Diff: http://git.reviewboard.kde.org/r/104515/diff/
> 
> 
> Testing
> ---
> 
> ecmascript & daily surfing
> 
> used valgrind on many array testcases to check for possible memleaks
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

Re: Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

(Updated April 9, 2012, 8:23 p.m.)


Review request for kdelibs.


Changes
---

Changed Array to use Object instead of Pointer to Objects.

This gives a 100ms speed boost compared to the Pointer version.
Overall its ~100ms slower than the original kjs 4.8.2, but that is expected as 
this version really checks for attributes.
So every put/delete/enum is really checked, a version without check is always 
faster.

Tested with sunspider 1.0 (latest svn trunk), both version run about 10 times 
(x10runs = 100).


I also tested virtual putDirect vs. non-virtual putDirect (only 
performancewise), the avg performance loss was 14ms (after 10 x 10 runs = 100 
sunspider 1.0 runs).
Anyway it does not seems like it is a very critical performance loss.


testregress showed a tiny/typo difference with max Array Index, this is fixed 
in this version. Now no difference when using
./testregression.shell --js --noxvfb ~/khtmltests/regression/
compared to the orginal kde 4.8.2 kjs


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs (updated)
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski

Review Request: KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

[Bernd Buschinski]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104515/
---

Review request for kdelibs.


Description
---

KJS: Implement Object.GetOwnPropertyDescriptor & Object.DefineProperty

This is a pretty big patch, to get Object.defineProperty perfect for ecmascript 
(for all tests that only use implemented stuff, all test that use Object.create 
for example will fail, as its not implemented)

PropertyDescriptor:
Necessary for collectiong data, this introduce new CommonIdentifiers.h, this 
might requiere to rebuild khtml against new kjs, otherwise it might cause weird 
crashes (at least for me)


object.h:
Beside from adding new getPropertyDescriptor & getOwnPropertyDescriptor & 
defineOwnProperty, the important changes are making
getPropertyAttributes, put/get/remove-Direct virtual.
Why do I need that?
Because put checks if the prototype already has property XYZ and uses it. Now 
imagine an array that got a setter-only property via a prototype. 
DefineProperty would try to use put, which uses the prototype property and it 
would fail. So all custom-data classes like Array need to implement/use 
put/get/remove-Direct.


object.cpp:
currently put on a setter-only property would always throw an exception, this 
is only correct for strict-mode, as we currently do not check for strict-mode 
it would make more sense to change it to default not throwing an exception.


array.cpp/h:
The old Array implementation did not store attributes for array indexes, I 
rewrote it to also store the attributes.
+ Bonus: also fix getOwnPropertyNames, as we now store attributes.
+ use new attributes, reject put/delete/enum if set

function.cpp (Arguments)
changed the default attributes how parameter are stored, according to ECMA 
10.6.11.b


Rest is "just" the defineProperty implementation


Diffs
-

  kjs/CMakeLists.txt 1188064 
  kjs/CommonIdentifiers.h 8ee40e8 
  kjs/array_instance.h 3f2b630 
  kjs/array_instance.cpp fe9b8b4 
  kjs/function.h 3757fe8 
  kjs/function.cpp 5f39ae6 
  kjs/object.h 047c242 
  kjs/object.cpp c19122f 
  kjs/object_object.cpp 986f03f 
  kjs/operations.h f8a28c8 
  kjs/operations.cpp d4c0066 

Diff: http://git.reviewboard.kde.org/r/104515/diff/


Testing
---

ecmascript & daily surfing

used valgrind on many array testcases to check for possible memleaks


Thanks,

Bernd Buschinski