Bug 151016 - create_hash_table should know whether a function is JSBuiltin or not.
Summary: create_hash_table should know whether a function is JSBuiltin or not.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-09 02:54 PST by youenn fablet
Modified: 2015-11-10 00:22 PST (History)
4 users (show)

See Also:


Attachments
Patch (34.55 KB, patch)
2015-11-09 03:19 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (34.66 KB, patch)
2015-11-09 23:32 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-11-09 02:54:18 PST
If create_hash_table script knew whether a function is JSBuiltin or not, we could do the following:
- Simplify lut generated code (removing #if JSC_BUILTIN_EXIST...)
- Simplify JS built-in generated code (removal JSC_BUILTIN_EXIST macros generation)
- Include "JSCBuiltins.h" in the lut generated code only if the table contains a JS built-in function.

This could also simplify the build system by removing the "-b" option of the create_hash_table script.
Comment 1 youenn fablet 2015-11-09 03:19:48 PST
Created attachment 265038 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-09 03:21:37 PST
This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)
Comment 3 youenn fablet 2015-11-09 05:29:15 PST
This patch does not yet simplify the build system.
This can be done in a follow-up bug, removing -b (and -i as well?).
Comment 4 BJ Burg 2015-11-09 09:32:31 PST
Comment on attachment 265038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265038&action=review

Good cleanup work!

> Source/JavaScriptCore/ChangeLog:14
> +        A further patch should simplify the build system by removing create_has_table -b option.

Nit: create_hash_table

> Source/JavaScriptCore/runtime/ArrayConstructor.cpp:54
> +  of        JSBuiltin                   DontEnum|Function 0

I would have preferred something that's an invalid C++ function name, like @builtin or something. But this is fine, I think.
Comment 5 youenn fablet 2015-11-09 23:32:18 PST
Created attachment 265149 [details]
Patch for landing
Comment 6 youenn fablet 2015-11-09 23:35:59 PST
> Good cleanup work!

Thanks :)

> > Source/JavaScriptCore/ChangeLog:14
> > +        A further patch should simplify the build system by removing create_has_table -b option.
> 
> Nit: create_hash_table

OK

> > Source/JavaScriptCore/runtime/ArrayConstructor.cpp:54
> > +  of        JSBuiltin                   DontEnum|Function 0
> 
> I would have preferred something that's an invalid C++ function name, like
> @builtin or something. But this is fine, I think.

Yes, it could be done. I kept JSBuiltin, as it is the same as the IDL keyword.
Also, in lut.h files, most of the function have names starting with lower case letters. It might be good to make the naming more consistent.
Comment 7 WebKit Commit Bot 2015-11-10 00:22:06 PST
Comment on attachment 265149 [details]
Patch for landing

Clearing flags on attachment: 265149

Committed r192204: <http://trac.webkit.org/changeset/192204>
Comment 8 WebKit Commit Bot 2015-11-10 00:22:11 PST
All reviewed patches have been landed.  Closing bug.