Summary: | number_object.cpp needs a bath | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | ||||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 16640 | ||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2007-12-28 11:12:36 PST
Created attachment 18145 [details]
Apply wkstyle/astyle and fix placement of *
JavaScriptCore/kjs/number_object.cpp | 695 +++++++++++++++++-----------------
JavaScriptCore/kjs/object.cpp | 3 +-
2 files changed, 339 insertions(+), 359 deletions(-)
Created attachment 18146 [details]
Break out callAsFunction implmentations into static functions
JavaScriptCore/kjs/number_object.cpp | 476 +++++++++++++++++-----------------
1 files changed, 244 insertions(+), 232 deletions(-)
Created attachment 18147 [details]
More small attempts to make number code readable
JavaScriptCore/kjs/number_object.cpp | 98 ++++++++++++++++++----------------
1 files changed, 51 insertions(+), 47 deletions(-)
Created attachment 18148 [details]
More small cleanups to toPrecision
JavaScriptCore/kjs/number_object.cpp | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
Created attachment 18149 [details]
More changes to make number code readable
JavaScriptCore/kjs/number_object.cpp | 44 +++++++++++++++++----------------
1 files changed, 23 insertions(+), 21 deletions(-)
I meant number_object.cpp not date_object! Comment on attachment 18146 [details]
Break out callAsFunction implmentations into static functions
It seems odd to me to use static functions here and not follow the lead of all the other classes and put each function in it's own class with its own callAsFunction. r-.
Comment on attachment 18145 [details]
Apply wkstyle/astyle and fix placement of *
Needs a changelog.
Comment on attachment 18147 [details]
More small attempts to make number code readable
Looks good, but again needs a changelog. r=me
Comment on attachment 18149 [details]
More changes to make number code readable
Looks good. I think this line should be a FIXME.
+ if (decimalPoint == 999) // ? 9999 is the magical "result is Inf or NaN" value. what's 999??
Comment on attachment 18146 [details]
Break out callAsFunction implmentations into static functions
After talking with Eric on IRC, he said he would provide a follow up patch to convert the static functions into their own classes to avoid the indirection.
Created attachment 18162 [details]
Move to modern multi-class function prototype system
JavaScriptCore/kjs/JSGlobalObject.cpp | 2 +-
JavaScriptCore/kjs/number_object.cpp | 79 ++++++++++++++++-----------------
JavaScriptCore/kjs/number_object.h | 20 ++++-----
3 files changed, 49 insertions(+), 52 deletions(-)
Comment on attachment 18162 [details]
Move to modern multi-class function prototype system
Sam should look at this. Particularly the use of the "length" parameter in the prototype constructors. I'm not really sure I did that part right.
SunSpider reports that all of the patches add up to a slowdown: ** TOTAL **: *1.004x as slow* 3875.1ms +/- 0.2% 3889.5ms +/- 0.2% significant I've not tested individually yet to see which patch(s) cause a slowdown. The slowdown seems to come entirely from the final patch. the "multi-class" prototype system. ** TOTAL **: - 3875.1ms +/- 0.2% 3871.7ms +/- 0.2% is the number w/o that patch. Running sunspider again, shows the total patch as a wash: ** TOTAL **: - 3875.1ms +/- 0.2% 3878.3ms +/- 0.2% So I think this is OK, even with the final "many class" patch. Comment on attachment 18162 [details]
Move to modern multi-class function prototype system
There are two things that worry me about this patch. The first is that a different functionPrototype object can be set since you are no longer passing in d()->functionPrototype but using the one off the execState (note, these may be the same thing). I am not sure this is an issue in practice, but it is a concern. The second issue is that by using the macro to make the class declarations for the functions, an unused create function is also being created. I am going to think about these two issues a little and do a more conclusive review later on tonight.
I've landed the reviewed patches. When sam decides on the last one I'll land or not land that. Comment on attachment 18162 [details]
Move to modern multi-class function prototype system
I don't see any point in landing this change if it is not a performance improvement. It will just add to the code size for no particular progression. r-
Sounds fine to me. |