Summary: | [Bindings] Declare dictionary / enumeration template specializations in the header | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, darin, ggaren, jiewen_tan, sam, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 162912 | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2016-10-04 12:15:30 PDT
Created attachment 290643 [details]
Patch
Created attachment 290645 [details]
Patch
Do the implementations really have to go in the header? Can't we just forward declare what we want to call? (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. (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. Created attachment 290665 [details]
Patch
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. (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)? (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. Created attachment 290688 [details]
Patch
Comment on attachment 290688 [details] Patch Clearing flags on attachment: 290688 Committed r206812: <http://trac.webkit.org/changeset/206812> All reviewed patches have been landed. Closing bug. |