Summary: | The generator and intrinsic fields in HashTableValue/HashEntry and associated structures and methods are redundant | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||
Component: | JavaScriptCore | Assignee: | Gavin Barraclough <barraclough> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | barraclough, gustavo, sam, xan.lopez | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Filip Pizlo
2011-09-18 15:40:50 PDT
If you are going to make this change, it might also be a good opportunity to remove the special casing from the generation script and put it in the lookup definitions. It may even be that we don't want the enum to be in either JIT - we may with to make use of the intrinsic from the interpreter, in environments where we don't have the JIT enabled. Maybe an enum in JSFunction? (In reply to comment #2) > It may even be that we don't want the enum to be in either JIT - we may with to make use of the intrinsic from the interpreter, in environments where we don't have the JIT enabled. Maybe an enum in JSFunction? How about runtime/Intrinsic.h? The benefit of a separate file is that: 1) JSFunction and pretty much everything else in runtime/ is cluttered. 2) If we do our jobs correctly then this enum will get big. ;-) Agreed, that sounds even better. Created attachment 117999 [details]
Fix
Comment on attachment 117999 [details]
Fix
r=me
Comment on attachment 117999 [details] Fix Attachment 117999 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10690698 Comment on attachment 117999 [details] Fix Attachment 117999 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10728902 Comment on attachment 117999 [details] Fix Attachment 117999 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10729969 Comment on attachment 117999 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=117999&action=review > Source/JavaScriptCore/create_hash_table:276 > if ($key eq "charCodeAt") { > $thunkGenerator = "charCodeAtThunkGenerator"; > - $intrinsic = "DFG::CharCodeAtIntrinsic"; > + $intrinsic = "CharCodeAtIntrinsic"; Do we really need this specialized check? Can't we just write CharCodeAtIntrinsic at the definition site? > Source/JavaScriptCore/create_hash_table:323 > $thunkGenerator = "logThunkGenerator"; > + $intrinsic = "LogIntrinsic"; Is $thunkGenerator still being used? (In reply to comment #10) > (From update of attachment 117999 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117999&action=review > > > Source/JavaScriptCore/create_hash_table:276 > > if ($key eq "charCodeAt") { > > $thunkGenerator = "charCodeAtThunkGenerator"; > > - $intrinsic = "DFG::CharCodeAtIntrinsic"; > > + $intrinsic = "CharCodeAtIntrinsic"; > > Do we really need this specialized check? Can't we just write CharCodeAtIntrinsic at the definition site? I definitely agree with you that we should do this, but I don't think it's a part of this change. This patch changes the data in the table output by the create_hash_table script, you're proposing changing the data in the tables parsed in by create_hash_table, these two changes are completely independent of each other. > > Source/JavaScriptCore/create_hash_table:323 > > $thunkGenerator = "logThunkGenerator"; > > + $intrinsic = "LogIntrinsic"; > > Is $thunkGenerator still being used? Ooops - meant to remove them - thanks! - will do. Fixed in r102167 |