Bug 97251 – GlobalObjectMethodTable initialization should use designated initializers

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Pratik Solanki]

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.

[Pratik Solanki]

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.

[Pratik Solanki]

Created attachment 164967 [details]
Patch

[Geoffrey Garen]

I like this, but I think older versions of GCC object. I guess we'll see...

[Gyuyoung Kim]

Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13949292

[Build Bot]

Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13948313

[Early Warning System Bot]

Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13958294

[Pratik Solanki]

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?

[Early Warning System Bot]

Comment on attachment 164967 [details]
Patch

Attachment 164967 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13949329

[Geoffrey Garen]

Is there a way to do that without having to write all the initializers twice?

[Sam Weinig]

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

[Sam Weinig]

(In reply to comment #9)
> Is there a way to do that without having to write all the initializers twice?

Macros of course!

[Sam Weinig]

Comment on attachment 164967 [details]
Patch

r-ing given how much this breaks.

[Pratik Solanki]

Created attachment 179151 [details]
Patch

[Pratik Solanki]

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.

[Darin Adler]

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?

[Pratik Solanki]

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

[Pratik Solanki]

This is probably not worth doing. Closing.

[Zan Dobersek]

Comment on attachment 179151 [details]
Patch

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