Bug 150491 – Teach create_hash_table to omit builtins macros when generating tables for native-only objects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Blaze Burg]

Also clean up some crap. This blocks separate compilation for JSC builtins.

[Blaze Burg]

Created attachment 263910 [details]
Proposed Fix

Might need a few rounds of EWS to get the CMake changes correct.

[Yusuke Suzuki]

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.

[youenn fablet]

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.

[Blaze Burg]

(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.

[youenn fablet]

> 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.

[Blaze Burg]

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.

[Blaze Burg]

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.

[WebKit Commit Bot]

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.

[Blaze Burg]

Committed r191537: <http://trac.webkit.org/changeset/191537>

[Blaze Burg]

Comment on attachment 263983 [details]
Fix CMake

Landed manually.