Bug 150536 – Builtins generator should emit ENABLE(FEATURE) guards based on @conditional annotation

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Blaze Burg]

patch.

[Blaze Burg]

Created attachment 263985 [details]
Patch

[WebKit Commit Bot]

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`)

[WebKit Commit Bot]

Attachment 263985 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Just a quick note that I used @optional in streams built-in js, but maybe this is a poor name.
WebIDL is using "Conditional" keyword.
Generated code is using "ENABLE" macro.

[Blaze Burg]

(In reply to comment #4)
> Just a quick note that I used @optional in streams built-in js, but maybe
> this is a poor name.
> WebIDL is using "Conditional" keyword.
> Generated code is using "ENABLE" macro.

In other code generators, it is along the lines of 

// @conditional=ENABLE(STREAMS)

so that files can be conditionalized on PLATFORM(), USE(), etc.
It is easy to change this.

[youenn fablet]

(In reply to comment #5)
> (In reply to comment #4)
> > Just a quick note that I used @optional in streams built-in js, but maybe
> > this is a poor name.
> > WebIDL is using "Conditional" keyword.
> > Generated code is using "ENABLE" macro.
> 
> In other code generators, it is along the lines of 
> 
> // @conditional=ENABLE(STREAMS)
> 
> so that files can be conditionalized on PLATFORM(), USE(), etc.
> It is easy to change this.

Sounds good to me

[Yusuke Suzuki]

Comment on attachment 263985 [details]
Patch

r=me, annotation looks nice ;)

[Blaze Burg]

(In reply to comment #7)
> Comment on attachment 263985 [details]
> Patch
> 
> r=me, annotation looks nice ;)

Before landing, as discussed above, I'll change @optional to @conditional and include ENABLE() in all the existing guards. I'll put up another patch for EWS to catch any mistakes.

[Yusuke Suzuki]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 263985 [details]
> > Patch
> > 
> > r=me, annotation looks nice ;)
> 
> Before landing, as discussed above, I'll change @optional to @conditional
> and include ENABLE() in all the existing guards. I'll put up another patch
> for EWS to catch any mistakes.

@conditional looks preferable!
So your revised patch will include the existing stream JS code, right? (replacing @optional to @conditional)

[Blaze Burg]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 263985 [details]
> > > Patch
> > > 
> > > r=me, annotation looks nice ;)
> > 
> > Before landing, as discussed above, I'll change @optional to @conditional
> > and include ENABLE() in all the existing guards. I'll put up another patch
> > for EWS to catch any mistakes.
> 
> @conditional looks preferable!
> So your revised patch will include the existing stream JS code, right?
> (replacing @optional to @conditional)

Yes.

[Blaze Burg]

Created attachment 264119 [details]
Proposed Fix

[WebKit Commit Bot]

Attachment 264119 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

LGTM.
Just wondering whether we also want to test "@conditional=ENABLE(STREAMS_API) || USE(CF)" annotation?

[Blaze Burg]

(In reply to comment #13)
> LGTM.
> Just wondering whether we also want to test
> "@conditional=ENABLE(STREAMS_API) || USE(CF)" annotation?

Sure. The regex should accept any non '=' character as the annotation's value. But would be good to verify.

[Blaze Burg]

Created attachment 264197 [details]
Proposed Fix

[WebKit Commit Bot]

Attachment 264197 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 264197 [details]
Proposed Fix

Setting cq+ with additional changes as discussed in bug.

[WebKit Commit Bot]

Comment on attachment 264197 [details]
Proposed Fix

Rejecting attachment 264197 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 264197, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/349230

[Blaze Burg]

Committed r191687: <http://trac.webkit.org/changeset/191687>