Bug 72855

Summary: JavaScriptCore: Mark all exported symbols in the header file automatically.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: JavaScriptCoreAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, gustavo.noronha, gustavo, kevino, levin+threading, mrowe, rakuco, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 74990, 76147    
Bug Blocks: 67852    
Attachments:
Description Flags
Patch
none
Patch
none
It's ready to review
gyuyoung.kim: commit-queue-
Patch
none
Patch for landing webkit.review.bot: commit-queue-

Description Hajime Morrita 2011-11-20 21:56:49 PST
I'm writing a tool for that:
https://github.com/omo/ListExportables
Will announce this effort in webkit-dev shortly.
Comment 1 Hajime Morrita 2011-11-21 00:22:19 PST
Created attachment 116040 [details]
Patch
Comment 2 Hajime Morrita 2011-11-21 00:23:07 PST
This is just a preliminary result. What should land would be generated  from the ToT at that timing.
Comment 3 Hajime Morrita 2011-11-21 04:09:35 PST
Created attachment 116064 [details]
Patch
Comment 4 Hajime Morrita 2011-11-21 04:11:09 PST
Comment on attachment 116064 [details]
Patch

Uploaded to the wrong bug...
Comment 5 Hajime Morrita 2011-11-27 23:02:15 PST
Created attachment 116700 [details]
It's ready to review
Comment 6 WebKit Review Bot 2011-11-27 23:05:15 PST
Attachment 116700 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/OpaqueJSString.h..." exit_code: 1

Last 3072 characters of output:
[readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/PropertyDescriptor.h:50:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/MainThread.h:49:  The parameter name "paused" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSLock.h:96:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/text/StringImpl.h:279:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSGlobalData.h:301:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ByteArray.h:86:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:136:  The parameter name "mutex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/SamplingTool.h:223:  Missing spaces around =  [whitespace/operators] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:79:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:80:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:81:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/JavaScriptCore/wtf/text/AtomicString.h:57:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "entry" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 25 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Hajime Morrita 2011-11-27 23:06:49 PST
Blocking bugs being removed, now this is ready for the review for applying the rewrite!
You can validate the build by applying the the preliminary patch on Bug 72854,
which disables export list.

Note that
- The rewrite is based on svn r101220.
- Reported style error is false positive. touched line already contained such errors.
Comment 8 Gyuyoung Kim 2011-11-27 23:19:34 PST
Comment on attachment 116700 [details]
It's ready to review

Attachment 116700 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10639428
Comment 9 Early Warning System Bot 2011-11-27 23:20:39 PST
Comment on attachment 116700 [details]
It's ready to review

Attachment 116700 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10643406
Comment 10 Hajime Morrita 2011-11-27 23:33:27 PST
Oops. I need to change WebCore/config.h... Filed at Bug 73191.
Comment 11 Mark Rowe (bdash) 2011-11-28 01:18:46 PST
The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.
Comment 12 Hajime Morrita 2011-11-28 03:03:22 PST
(In reply to comment #11)
> The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.

Basically I agree with you that forcing JS_INLINE is ugly.

Fortunately, JS_INLINE will be used in limited places:
We need it only for inline method whose class has exported constructors and a vtable.

Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline.
I can help such inline to non-inline conversion if you are OK.

We need JS_INLINE (explicit "hidden" visibility annotation) because
there is no way to make vtables visible using source annotation except marking classes as visible.

I think this is a reasonable compromise compared to using export lists.
But I'd love to hear your suggestion.

Yet another idea is to #define inline ...
Comment 13 Collabora GTK+ EWS bot 2011-11-29 09:30:22 PST
Comment on attachment 116700 [details]
It's ready to review

Attachment 116700 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10678739
Comment 14 David Levin 2011-11-29 09:42:12 PST
(In reply to comment #12)
> (In reply to comment #11)
> > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.
> 
> Basically I agree with you that forcing JS_INLINE is ugly.
> 
> Fortunately, JS_INLINE will be used in limited places:
> We need it only for inline method whose class has exported constructors and a vtable.
> 
> Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline.
> I can help such inline to non-inline conversion if you are OK.
> 
> We need JS_INLINE (explicit "hidden" visibility annotation) because
> there is no way to make vtables visible using source annotation except marking classes as visible.
> 
> I think this is a reasonable compromise compared to using export lists.
> But I'd love to hear your suggestion.
> 
> Yet another idea is to #define inline ...

Does the build break if I get it wrong? (I would consider this a nice feature.)

In order words, how can we help people to get this right? (Both when to include it and when not to include it.)

PS I love the idea!
Comment 15 Hajime Morrita 2011-12-04 18:16:12 PST
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.
> > 
> > Basically I agree with you that forcing JS_INLINE is ugly.
> > 
> > Fortunately, JS_INLINE will be used in limited places:
> > We need it only for inline method whose class has exported constructors and a vtable.
> > 
> > Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline.
> > I can help such inline to non-inline conversion if you are OK.
> > 
> > We need JS_INLINE (explicit "hidden" visibility annotation) because
> > there is no way to make vtables visible using source annotation except marking classes as visible.
> > 
> > I think this is a reasonable compromise compared to using export lists.
> > But I'd love to hear your suggestion.
> > 
> > Yet another idea is to #define inline ...
> 
> Does the build break if I get it wrong? (I would consider this a nice feature.)
> 
> In order words, how can we help people to get this right? (Both when to include it and when not to include it.)
> 
Oops. Sorry for the slow response...
For Mac the build will simply fail because of the checker script.
For Chromium, maybe we can improve our clang plugin to detect this.
For other port, I have no idea. I guess it's possible to write a checker script which is similar to Mac's,
but I'm not sure. Fortunately the EWS has chromium bot so it would help other port developers.

> PS I love the idea!
Thanks!
Comment 16 Hajime Morrita 2011-12-11 21:29:28 PST
Comment on attachment 116700 [details]
It's ready to review

clearing r? until the blocking bugs are fixed.
Comment 17 Hajime Morrita 2012-01-11 00:08:55 PST
Created attachment 121986 [details]
Patch
Comment 18 WebKit Review Bot 2012-01-11 00:12:00 PST
Attachment 121986 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/OpaqueJSString.h..." exit_code: 1

Last 3072 characters of output:
ty/parameter_name] [5]
Source/JavaScriptCore/runtime/Identifier.h:123:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:54:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/PropertyDescriptor.h:55:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/MainThread.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/wtf/MainThread.h:52:  The parameter name "paused" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSLock.h:96:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/text/StringImpl.h:278:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSGlobalData.h:315:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/ByteArray.h:86:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:124:  The parameter name "mutex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/SamplingTool.h:223:  Missing spaces around =  [whitespace/operators] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:85:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:86:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:87:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/JavaScriptCore/wtf/text/AtomicString.h:57:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "entry" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:78:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 24 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Hajime Morrita 2012-01-11 00:13:37 PST
Finally, the patch is ready!
I confirmed that this can be buit even without exp file on SL (The exp list removal patch is on Bug 72854)
Thanks for the clean API design of JSC, we don't need any extra INLINE macros.

After landing this change, I'll attack both exp file (on Mac) and def file (on Windows).

Note that style red is false positive. That has been there before this change.
Comment 20 WebKit Review Bot 2012-01-11 00:40:58 PST
Comment on attachment 121986 [details]
Patch

Attachment 121986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11207108
Comment 21 Hajime Morrita 2012-01-11 17:09:49 PST
Darin, thanks you for the review!
I'll fix the chromium bot problem (it should be done by a separate patch) then land this.
Comment 22 Hajime Morrita 2012-01-12 22:01:17 PST
Created attachment 122369 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-01-12 22:04:30 PST
Comment on attachment 122369 [details]
Patch for landing

Rejecting attachment 122369 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ring.h
patching file Source/JavaScriptCore/wtf/text/StringBuilder.h
patching file Source/JavaScriptCore/wtf/text/StringImpl.h
patching file Source/JavaScriptCore/wtf/text/WTFString.h
patching file Source/JavaScriptCore/wtf/unicode/Collator.h
patching file Source/JavaScriptCore/wtf/unicode/UTF8.h
patching file Source/JavaScriptCore/yarr/Yarr.h
patching file Source/JavaScriptCore/yarr/YarrPattern.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/11142891
Comment 24 Hajime Morrita 2012-01-12 23:24:26 PST
Committed r104900: <http://trac.webkit.org/changeset/104900>