Bug 150491

Summary: Teach create_hash_table to omit builtins macros when generating tables for native-only objects
Product: WebKit Reporter: BJ Burg <bburg>
Component: JavaScriptCoreAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, ggaren, saam, youennf, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 150482    
Attachments:
Description Flags
Proposed Fix
none
Fix CMake none

Description BJ Burg 2015-10-22 23:01:02 PDT
Also clean up some crap. This blocks separate compilation for JSC builtins.
Comment 1 BJ Burg 2015-10-22 23:55:58 PDT
Created attachment 263910 [details]
Proposed Fix

Might need a few rounds of EWS to get the CMake changes correct.
Comment 2 Yusuke Suzuki 2015-10-23 09:12:17 PDT
Comment on attachment 263910 [details]
Proposed Fix

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

:D r=me with very small nit.

> Source/JavaScriptCore/create_hash_table:325
>          if ($values[$i]{"type"} eq "Function")  {

I think making this as `$values[$i]{"type"} eq "Function" && $includeBuiltin` is preferable since it maintains consistency between the upper one and lower one.
Or defining a variable like `my $mayBeBuildinFunction = $values[$i]{"type"} eq "Function" && $includeBuiltin` and using it may be nice.
Comment 3 youenn fablet 2015-10-23 10:37:37 PDT
I am not very familiar with lut use and generation. Sorry if some of the points below are not useful.

The current way of using JS built-ins through lut information in CPP files seems a bit complex to me.
Wouldn't it be easier if the lut info was more explicit about whether a function is implemented as built-in or not?
That would be clearer to the reader. This would remove the need for JSC_BUILTIN_EXISTS. This may help the script become smarter.

In the scope of that particular bug, it seems easier for me to include "XXXBuiltins.h" in a CPP file than to update the two build systems when a lut info starts to use built-ins.
Comment 4 BJ Burg 2015-10-23 11:07:41 PDT
(In reply to comment #3)
> I am not very familiar with lut use and generation. Sorry if some of the
> points below are not useful.
> 
> The current way of using JS built-ins through lut information in CPP files
> seems a bit complex to me.
> Wouldn't it be easier if the lut info was more explicit about whether a
> function is implemented as built-in or not?
> That would be clearer to the reader. This would remove the need for
> JSC_BUILTIN_EXISTS. This may help the script become smarter.

That could work.

> In the scope of that particular bug, it seems easier for me to include
> "XXXBuiltins.h" in a CPP file than to update the two build systems when a
> lut info starts to use built-ins.

If XXX is JSC, then touching any builtin.js file will cause all files that include a .lut.h (and have a builtin) to rebuild. If XXX is the specific builtin, then the generator needs to know which inputs have builtins otherwise it will include a non-existent header for those without.

If the lut input format is changed to include builtin/native, then we can remove the guards and includes altogether. But I think this is an easier first step towards separate compilation than that.
Comment 5 youenn fablet 2015-10-23 13:08:43 PDT
> If XXX is the specific
> builtin, then the generator needs to know which inputs have builtins
> otherwise it will include a non-existent header for those without.

Or the generator can include no header at all, leaving that work to the the cpp file that includes the .lut.h.
No big difference really, except maybe that the explicit include is a hint for cautious readers that built-ins are in action.
Comment 6 BJ Burg 2015-10-23 14:51:50 PDT
I am investigating the CMake build failures, but haven't figured it out yet. It looks like this change caused NativeErrorPrototype.cpp to not see a definition for class ErrorPrototype. I didn't change the headers However all the .lut.h files seem to generated correctly. There might be some trouble with the new create_hash_tables invocation.
Comment 7 BJ Burg 2015-10-24 13:19:07 PDT
Created attachment 263983 [details]
Fix CMake

Required a bit of hackery to pass flags to a macro in CMake. But it seems to work now.
Comment 8 WebKit Commit Bot 2015-10-24 13:21:20 PDT
Attachment 263983 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/CMakeLists.txt:1029:  No space before ")"  [whitespace/parentheses] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 BJ Burg 2015-10-24 14:40:21 PDT
Committed r191537: <http://trac.webkit.org/changeset/191537>
Comment 10 BJ Burg 2015-10-24 14:40:44 PDT
Comment on attachment 263983 [details]
Fix CMake

Landed manually.