Bug 97251 - GlobalObjectMethodTable initialization should use designated initializers
Summary: GlobalObjectMethodTable initialization should use designated initializers
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-20 12:44 PDT by Pratik Solanki
Modified: 2013-09-05 08:35 PDT (History)
6 users (show)

See Also:

Attachments
Patch (4.15 KB, patch)
2012-09-20 12:53 PDT, Pratik Solanki 		no flags Details | Formatted Diff | Diff
Patch (4.95 KB, text/plain)
2012-12-12 16:50 PST, Pratik Solanki 		no flags Details
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2012-09-20 12:44:46 PDT 
Instead of doing

const GlobalObjectMethodTable JSDOMWindowBase::s_globalObjectMethodTable = { &shouldAllowAccessFrom, &supportsProfiling, &supportsRichSourceInfo, &shouldInterruptScript, &javaScriptExperimentsEnabled };

and relying on the order in which the fields are written, we can use designated initializers.
Comment 1 Pratik Solanki 2012-09-20 12:45:34 PDT 
I'm not sure if we have precedent for using designated initializers in webkit. I'll upload patch for review and see what EWS says.
Comment 2 Pratik Solanki 2012-09-20 12:53:37 PDT 
Created attachment 164967 [details]
Patch
Comment 3 Geoffrey Garen 2012-09-20 12:55:28 PDT 
I like this, but I think older versions of GCC object. I guess we'll see...
Comment 4 Gyuyoung Kim 2012-09-20 13:00:54 PDT 
Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13949292
Comment 5 Build Bot 2012-09-20 13:05:19 PDT 
Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13948313
Comment 6 Early Warning System Bot 2012-09-20 14:09:05 PDT 
Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13958294
Comment 7 Pratik Solanki 2012-09-20 14:30:31 PDT 
Ok. This isn't going to work due to failures in other ports. What if I added a HAVE(DESIGNATED_INITIALIZERS) macro and move the code there? And then we can turn it on for platforms that support it. Is it worth doing that?
Comment 8 Early Warning System Bot 2012-09-20 15:08:54 PDT 
Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13949329
Comment 9 Geoffrey Garen 2012-09-20 21:05:50 PDT 
Is there a way to do that without having to write all the initializers twice?
Comment 10 Sam Weinig 2012-09-23 00:04:18 PDT 
(In reply to comment #7)
> Ok. This isn't going to work due to failures in other ports. What if I added a HAVE(DESIGNATED_INITIALIZERS) macro and move the code there? And then we can turn it on for platforms that support it. Is it worth doing that?

If you do end up doing it with a #if check, please use a COMPILER_SUPPORTS() macro instead of HAVE().
Comment 11 Sam Weinig 2012-09-23 00:05:09 PDT 
(In reply to comment #9)
> Is there a way to do that without having to write all the initializers twice?

Macros of course!
Comment 12 Sam Weinig 2012-09-25 00:55:20 PDT 
Comment on attachment 164967 [details]
Patch

r-ing given how much this breaks.
Comment 13 Pratik Solanki 2012-12-12 16:50:40 PST 
Created attachment 179151 [details]
Patch
Comment 14 Pratik Solanki 2012-12-12 16:52:15 PST 
Couldn't figure out a way to avoid the code duplication. I'm enabling the code for clang only. I'm sure newer versions of gcc support designated initializers but I didn't test. Anyone who compiles with gcc can test and enable the code path on that compiler.
Comment 15 Darin Adler 2012-12-12 20:03:32 PST 
Comment on attachment 179151 [details]
Patch

Given that we can use this only in certain compilers, and still have to keep the #else branch working, this doesn’t seem worth doing to me. Does this solve a concrete problem?
Comment 16 Pratik Solanki 2012-12-12 23:32:13 PST 
(In reply to comment #15)
> (From update of attachment 179151 [details])
> Given that we can use this only in certain compilers, and still have to keep the #else branch working, this doesn’t seem worth doing to me. Does this solve a concrete problem?

This was motivated by <rdar://12329156>. Having designated initializers would have prevented that. But I do I agree this does add more complexity. Is there anything better we can do?
Comment 17 Pratik Solanki 2013-05-03 12:28:56 PDT 
This is probably not worth doing. Closing.
Comment 18 Zan Dobersek 2013-09-05 08:35:09 PDT 
Comment on attachment 179151 [details]
Patch

The work on this bug has ceased, removing the r? flag on the patch.