Bug 162929 - [Bindings] Declare dictionary / enumeration template specializations in the header
Summary: [Bindings] Declare dictionary / enumeration template specializations in the h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 162912
  Show dependency treegraph
 
Reported: 2016-10-04 12:15 PDT by Chris Dumez
Modified: 2016-10-05 09:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (103.09 KB, patch)
2016-10-04 14:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (103.16 KB, patch)
2016-10-04 14:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (94.59 KB, patch)
2016-10-04 16:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (30.60 KB, patch)
2016-10-04 22:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-10-04 12:15:30 PDT
Move convertDictionary<>() template specializations to the header in order to:
- Allow them to be used from custom bindings code.
- Prepare for moving dictionaries to their own IDL files (Bug 162912).
Comment 1 Chris Dumez 2016-10-04 14:23:06 PDT
Created attachment 290643 [details]
Patch
Comment 2 Chris Dumez 2016-10-04 14:37:42 PDT
Created attachment 290645 [details]
Patch
Comment 3 Sam Weinig 2016-10-04 15:37:38 PDT
Do the implementations really have to go in the header? Can't we just forward declare what we want to call?
Comment 4 Chris Dumez 2016-10-04 15:45:27 PDT
(In reply to comment #3)
> Do the implementations really have to go in the header? Can't we just
> forward declare what we want to call?

Your comment only applies to jsStringWithCache() right? All other functions are templated so I believe they will need to be defined in the header if we want them to be callable from other compilation units.

I do think we can move the definition of jsStringWithCache() to the cpp though and only keep its declaration in the header.
Comment 5 Chris Dumez 2016-10-04 15:47:05 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Do the implementations really have to go in the header? Can't we just
> > forward declare what we want to call?
> 
> Your comment only applies to jsStringWithCache() right? All other functions
> are templated so I believe they will need to be defined in the header if we
> want them to be callable from other compilation units.
> 
> I do think we can move the definition of jsStringWithCache() to the cpp
> though and only keep its declaration in the header.

Although I don't know about the performance impact of doing so given that the function was likely being inlined.
Comment 6 Chris Dumez 2016-10-04 16:27:13 PDT
Created attachment 290665 [details]
Patch
Comment 7 Chris Dumez 2016-10-04 16:31:00 PDT
I have moved jsStringWithCache() implementation back to the cpp. I haven't tried instantiating the template functions in the header and moving the template specializations to the cpp yet.
Comment 8 Chris Dumez 2016-10-04 16:34:18 PDT
(In reply to comment #7)
> I haven't
> tried instantiating the template functions in the header and moving the
> template specializations to the cpp yet.

The thing is that I worry it will lead to linking errors when actually calling the specialization from another compilation unit (which we do not do yet so I won't notice) or about getting weird compilation trouble on Windows.

When I have tried tricks like this in the past, it usually hasn't gone well :/

Do you mind if we try doing so in a follow-up, once we actually start sharing dictionaries (i.e. after Bug 161932 is fixed)?
Comment 9 Chris Dumez 2016-10-04 20:54:26 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > I haven't
> > tried instantiating the template functions in the header and moving the
> > template specializations to the cpp yet.
> 
> The thing is that I worry it will lead to linking errors when actually
> calling the specialization from another compilation unit (which we do not do
> yet so I won't notice) or about getting weird compilation trouble on Windows.
> 
> When I have tried tricks like this in the past, it usually hasn't gone well
> :/
> 
> Do you mind if we try doing so in a follow-up, once we actually start
> sharing dictionaries (i.e. after Bug 161932 is fixed)?

Oh, it looks like it is easier than I thought. I'll try and do it in this patch.
Comment 10 Chris Dumez 2016-10-04 22:04:13 PDT
Created attachment 290688 [details]
Patch
Comment 11 Chris Dumez 2016-10-05 09:11:50 PDT
Comment on attachment 290688 [details]
Patch

Clearing flags on attachment: 290688

Committed r206812: <http://trac.webkit.org/changeset/206812>
Comment 12 Chris Dumez 2016-10-05 09:11:56 PDT
All reviewed patches have been landed.  Closing bug.