Use of pimpl idiom for Ewk classes
Hi, Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes. The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods. At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly. We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented. I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696 I also wrote a first patch to *show what the result looks* like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: *https://bugs.webkit.org/show_bug.cgi?id=99697* We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
Hi there, This sounds really nice :-) and is what Qt has been doing for long, so I am very supportive of this effort. Deos this mean that we can get rid of all of the private C methods? Kenneth On Thu, Oct 18, 2012 at 10:56 AM, Dumez, Christophe <christophe.dumez@intel.com> wrote:
Hi,
Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes.
The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods.
At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly.
We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented.
I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696
I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697
We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style.
Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
_______________________________________________ webkit-efl mailing list webkit-efl@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-efl
-- Kenneth Rohde Christiansen Senior Engineer, WebKit, Qt, EFL Phone +45 4093 0598 / E-mail kenneth at webkit.org ﹆﹆﹆
On Oct 18, 2012, at 12:25, Kenneth Rohde Christiansen <kenneth.christiansen@gmail.com> wrote:
Hi there,
This sounds really nice :-) and is what Qt has been doing for long, so I am very supportive of this effort.
Deos this mean that we can get rid of all of the private C methods?
Yes. Please check my patch. You will see we don't need these private C functions anymore.
Hi Chris, FOA, thanks for the interesting topic here. :)
From this, I've learned about useful design pattern and implementation technique with respect to encapsulation in C++.
I personally believe things have two aspects, e.g. pros and cons. :) Here pros is strongly clear. We can save the word for encapsulation. Then, now my question is what's the cons from this change? It seems QT project widely uses this kind of technique called D-pointer. Any report from them? Kind regards, Kangil From: webkit-efl-bounces@lists.webkit.org [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Dumez, Christophe Sent: Thursday, October 18, 2012 5:56 PM To: webkit-efl@lists.webkit.org Cc: Kenneth R Christiansen Subject: [webkit-efl] Use of pimpl idiom for Ewk classes Hi, Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes. The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods. At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly. We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented. I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696 I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697 We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
Hi Kangil, Well first of all you have to strictly follow the pattern and not to forget putting macros where they are supposed to be, It might be confusing from the beginning. Chris and me however promise to describe the approach in the EFL coding style wiki. Another drawback is passing calls through an extra pointer because of pimpl, so performance can be affected in theory. BR, Mikhail From: webkit-efl-bounces@lists.webkit.org [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Kangil Han Sent: Thursday, October 18, 2012 4:58 PM To: Dumez, Christophe; webkit-efl@lists.webkit.org Cc: Christiansen, Kenneth R Subject: Re: [webkit-efl] Use of pimpl idiom for Ewk classes Hi Chris, FOA, thanks for the interesting topic here. :)
From this, I've learned about useful design pattern and implementation technique with respect to encapsulation in C++.
I personally believe things have two aspects, e.g. pros and cons. :) Here pros is strongly clear. We can save the word for encapsulation. Then, now my question is what's the cons from this change? It seems QT project widely uses this kind of technique called D-pointer. Any report from them? Kind regards, Kangil From: webkit-efl-bounces@lists.webkit.org<mailto:webkit-efl-bounces@lists.webkit.org> [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Dumez, Christophe Sent: Thursday, October 18, 2012 5:56 PM To: webkit-efl@lists.webkit.org<mailto:webkit-efl@lists.webkit.org> Cc: Kenneth R Christiansen Subject: [webkit-efl] Use of pimpl idiom for Ewk classes Hi, Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes. The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods. At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly. We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented. I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696 I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697 We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Hi Mikhail, I think you mentioned two points. First of all, developers should get used at the design pattern. ;) Then, we will have extra pointer operation. Thanks for the kind feedback! From: Pozdnyakov, Mikhail [mailto:mikhail.pozdnyakov@intel.com] Sent: Thursday, October 18, 2012 11:10 PM To: kangil.han@samsung.com; Dumez, Christophe; webkit-efl@lists.webkit.org Cc: Christiansen, Kenneth R Subject: RE: [webkit-efl] Use of pimpl idiom for Ewk classes Hi Kangil, Well first of all you have to strictly follow the pattern and not to forget putting macros where they are supposed to be, It might be confusing from the beginning. Chris and me however promise to describe the approach in the EFL coding style wiki. Another drawback is passing calls through an extra pointer because of pimpl, so performance can be affected in theory. BR, Mikhail From: webkit-efl-bounces@lists.webkit.org [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Kangil Han Sent: Thursday, October 18, 2012 4:58 PM To: Dumez, Christophe; webkit-efl@lists.webkit.org Cc: Christiansen, Kenneth R Subject: Re: [webkit-efl] Use of pimpl idiom for Ewk classes Hi Chris, FOA, thanks for the interesting topic here. :)
From this, I've learned about useful design pattern and implementation technique with respect to encapsulation in C++.
I personally believe things have two aspects, e.g. pros and cons. :) Here pros is strongly clear. We can save the word for encapsulation. Then, now my question is what's the cons from this change? It seems QT project widely uses this kind of technique called D-pointer. Any report from them? Kind regards, Kangil From: webkit-efl-bounces@lists.webkit.org [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Dumez, Christophe Sent: Thursday, October 18, 2012 5:56 PM To: webkit-efl@lists.webkit.org Cc: Kenneth R Christiansen Subject: [webkit-efl] Use of pimpl idiom for Ewk classes Hi, Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes. The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods. At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly. We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented. I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696 I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697 We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Thu, Oct 18, 2012 at 11:56 AM, Dumez, Christophe <christophe.dumez@intel.com> wrote:
Hi,
Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes.
The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods.
At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly.
We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented.
I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696
I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697
We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style.
Is it really necessary to make this a Qt-like pimpl? AFAIK Qt has this opaque d-pointer to keep ABI compatibility across releases. Since we are working on private code, I don't see need for this. Will just complexity to the code and extra pointers dereferences. Don't get me wrong, the refactoring is indeed necessary (but can be done differently). What I'm questioning here is the need of these macros and the "d->private" idiom. The change bellow is fine, removed C-ism from the code: - uint64_t downloadId = ewk_download_job_id_get(ewkDownload); + uint64_t downloadId = ewkDownload->id(); But I would write Ewk_Download_Job::id() like this: uint64_t Ewk_Download_Job::id() const { return m_downloadProxyId; } Instead of: uint64_t Ewk_Download_Job::id() const { return d->downloadProxy->downloadID(); } Why not instead of creating a Ewk_Download_Job_Data you store the information you need inside Ewk_Download_Job and mark as private attributes? Cheers,
Hi Thiago, what you are proposing (make the members private and add getters / setters for them) is the first thing I have tried. However, this results in a lot of more changes. 1. With the pimpl way, we can simply move the members to the cpp file in a POD type (Ok, with some macros). C API implementation remains mostly unchanged. 2. With the alternative approach, we will need a lot of new methods in the ewk classes to match the functionality provided by the public EFL/C API. And our cpp file is going to contain C functions and C++ methods implementations for the same functionality (the C functions calling the corresponding C++ methods). Mikhail said he would give the alternative approach a try for Ewk_Download so that we can compare the results. Hopefully, we can better see the difference then. Performance-wise, I don't see much difference. With pimpl, we have an extra pointer dereferencing in the C functions and with the alternative, we have an extra function call. BTW, Thiago, in your code, the example you are showing is for class methods but you need to take into consideration the C functions as well to have a full picture. Also, to be a fair comparison, your code example for the C++ method should have been: uint64_t Ewk_Download_Job::id() const { // return m_downloadProxyId; return m_downloadProxy->downloadID(); } Instead of: uint64_t Ewk_Download_Job::id() const { // return d->downloadProxyId; return d->downloadProxy->downloadID(); } (You created a new m_downloadProxyId member for one code sample but not the other ;) Kr, On Thu, Oct 18, 2012 at 6:08 PM, Thiago Marcos P. Santos < tmpsantos@gmail.com> wrote:
On Thu, Oct 18, 2012 at 11:56 AM, Dumez, Christophe <christophe.dumez@intel.com> wrote:
Hi,
Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes.
The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods.
At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly.
We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented.
I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696
I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697
We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style.
Is it really necessary to make this a Qt-like pimpl?
AFAIK Qt has this opaque d-pointer to keep ABI compatibility across releases. Since we are working on private code, I don't see need for this. Will just complexity to the code and extra pointers dereferences.
Don't get me wrong, the refactoring is indeed necessary (but can be done differently). What I'm questioning here is the need of these macros and the "d->private" idiom. The change bellow is fine, removed C-ism from the code:
- uint64_t downloadId = ewk_download_job_id_get(ewkDownload); + uint64_t downloadId = ewkDownload->id();
But I would write Ewk_Download_Job::id() like this:
uint64_t Ewk_Download_Job::id() const { return m_downloadProxyId; }
Instead of:
uint64_t Ewk_Download_Job::id() const { return d->downloadProxy->downloadID(); }
Why not instead of creating a Ewk_Download_Job_Data you store the information you need inside Ewk_Download_Job and mark as private attributes?
Cheers,
-- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
Hi, I have updated the Ewk_Download class based on the alternative mentioned by Thiago and updated a patch here for comparison: https://bugs.webkit.org/attachment.cgi?id=169425&action=prettypatch (pimpl patch is still at https://bugs.webkit.org/attachment.cgi?id=169402&action=prettypatch) One possible issue that I noticed with this approach is that we need to use "const char*" instead of "String" type in our C++ methods API due to Eina string sharing. This is a bit unfortunate since C++ code uses String type in WebKit. As I said in my previous email, we also need to write a lot more C++ methods than with the pimpl approach. It will be a lot more work to refactor all Ewk classes. This approach however has the benefit of being of simpler design (no macros, C++ methods can access members directly without dereferencing a pointer). I'm personally open to both approaches so I'd like to hear others' opinion. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
Hi, Pimpl has its advantages, however think we should come up to the solution where all the object functionality is encapsulated in one C++ class exposing both private and public interfaces and C API functions are just wrapping around class methods. Think it is simpler and looks more natural. So approach #2 I guess.. BR, Mikhail ________________________________________ From: webkit-efl-bounces@lists.webkit.org [webkit-efl-bounces@lists.webkit.org] on behalf of Dumez, Christophe [christophe.dumez@intel.com] Sent: Thursday, October 18, 2012 7:27 PM To: Thiago Marcos P. Santos Cc: webkit-efl@lists.webkit.org; Christiansen, Kenneth R Subject: Re: [webkit-efl] Use of pimpl idiom for Ewk classes Hi, I have updated the Ewk_Download class based on the alternative mentioned by Thiago and updated a patch here for comparison: https://bugs.webkit.org/attachment.cgi?id=169425&action=prettypatch (pimpl patch is still at https://bugs.webkit.org/attachment.cgi?id=169402&action=prettypatch) One possible issue that I noticed with this approach is that we need to use "const char*" instead of "String" type in our C++ methods API due to Eina string sharing. This is a bit unfortunate since C++ code uses String type in WebKit. As I said in my previous email, we also need to write a lot more C++ methods than with the pimpl approach. It will be a lot more work to refactor all Ewk classes. This approach however has the benefit of being of simpler design (no macros, C++ methods can access members directly without dereferencing a pointer). I'm personally open to both approaches so I'd like to hear others' opinion. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Hi Chris, FOA, thank you, of course including Mikhail, for the constructive discussion. :) My two cents heads to https://bugs.webkit.org/attachment.cgi?id=169425 <https://bugs.webkit.org/attachment.cgi?id=169425&action=prettypatch> &action=prettypatch No macro and so pretty! Kind regards, Kangil From: webkit-efl-bounces@lists.webkit.org [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Dumez, Christophe Sent: Friday, October 19, 2012 1:28 AM To: Thiago Marcos P. Santos Cc: webkit-efl@lists.webkit.org; Kenneth R Christiansen Subject: Re: [webkit-efl] Use of pimpl idiom for Ewk classes Hi, I have updated the Ewk_Download class based on the alternative mentioned by Thiago and updated a patch here for comparison: https://bugs.webkit.org/attachment.cgi?id=169425 <https://bugs.webkit.org/attachment.cgi?id=169425&action=prettypatch> &action=prettypatch (pimpl patch is still at https://bugs.webkit.org/attachment.cgi?id=169402 <https://bugs.webkit.org/attachment.cgi?id=169402&action=prettypatch> &action=prettypatch) One possible issue that I noticed with this approach is that we need to use "const char*" instead of "String" type in our C++ methods API due to Eina string sharing. This is a bit unfortunate since C++ code uses String type in WebKit. As I said in my previous email, we also need to write a lot more C++ methods than with the pimpl approach. It will be a lot more work to refactor all Ewk classes. This approach however has the benefit of being of simpler design (no macros, C++ methods can access members directly without dereferencing a pointer). I'm personally open to both approaches so I'd like to hear others' opinion. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
One possible issue that I noticed with this approach is that we need to use "const char*" instead of "String" type in our C++ methods API due to Eina string sharing. This is a bit unfortunate since C++ code uses String type in WebKit.
Btw, what does this mean? Would you please give us an example code? Many thanks, Kangil From: Kangil Han [mailto:kangil.han@samsung.com] Sent: Friday, October 19, 2012 5:50 PM To: 'Dumez, Christophe'; 'Thiago Marcos P. Santos' Cc: 'webkit-efl@lists.webkit.org'; 'Kenneth R Christiansen' Subject: RE: [webkit-efl] Use of pimpl idiom for Ewk classes Hi Chris, FOA, thank you, of course including Mikhail, for the constructive discussion. :) My two cents heads to https://bugs.webkit.org/attachment.cgi?id=169425 <https://bugs.webkit.org/attachment.cgi?id=169425&action=prettypatch> &action=prettypatch No macro and so pretty! Kind regards, Kangil From: webkit-efl-bounces@lists.webkit.org [mailto:webkit-efl-bounces@lists.webkit.org] On Behalf Of Dumez, Christophe Sent: Friday, October 19, 2012 1:28 AM To: Thiago Marcos P. Santos Cc: webkit-efl@lists.webkit.org; Kenneth R Christiansen Subject: Re: [webkit-efl] Use of pimpl idiom for Ewk classes Hi, I have updated the Ewk_Download class based on the alternative mentioned by Thiago and updated a patch here for comparison: https://bugs.webkit.org/attachment.cgi?id=169425 <https://bugs.webkit.org/attachment.cgi?id=169425&action=prettypatch> &action=prettypatch (pimpl patch is still at https://bugs.webkit.org/attachment.cgi?id=169402 <https://bugs.webkit.org/attachment.cgi?id=169402&action=prettypatch> &action=prettypatch) One possible issue that I noticed with this approach is that we need to use "const char*" instead of "String" type in our C++ methods API due to Eina string sharing. This is a bit unfortunate since C++ code uses String type in WebKit. As I said in my previous email, we also need to write a lot more C++ methods than with the pimpl approach. It will be a lot more work to refactor all Ewk classes. This approach however has the benefit of being of simpler design (no macros, C++ methods can access members directly without dereferencing a pointer). I'm personally open to both approaches so I'd like to hear others' opinion. Kr, -- Christophe Dumez Linux Software Engineer, PhD Intel Finland Oy - Open Source Technology Center
Performance-wise, I don't see much difference. With pimpl, we have an extra pointer dereferencing in the C functions and with the alternative, we have an extra function call.
Actually the function call should be more expensive than a simple dereferencing. With a private class it also encapsulates the private code quite nicely. Kenneth
On Thu, Oct 18, 2012 at 11:56 AM, Dumez, Christophe <christophe.dumez@intel.com> wrote:
Hi,
Mikhail and I are currently working on refactoring the Ewk classes to make them more C++-like, more convenience to use and less bug-prone. As part of this work, we came out with a proposal: Use pimpl idom for Ewk classes.
The Ewk classes are currently defined in the private headers, with all their members public. The private headers also usually contain C-style functions to interact with the Ewk class from inside WebKit. Now, we would like to get rid of those private C functions and replace them with C++ methods in the Ewk class. The issue is that the Ewk class members are currently public meaning we can alter the members directly from outside the Ewk class implementation and bypass those class methods.
At the same time, we don't want to make all the Ewk class members private and create getter / setter methods for all of them. Not only this means a lot of refactoring work and extra code, but also, this means that are public C API implementation will need to go through those getters / setters as well, instead of interacting with the class members directly.
We therefore thought of using the pimpl idiom so that the class members are now stored in a POD struct in the cpp file. This effectively prevents access to them from code other than our public C API and Ewk class methods, which is what we want. The C API functions can still directly interact with the Ewk class members which is convenient. We can get rid of all C functions in the private headers and replace them with Ewk class methods. Therefore, the coding style in WK2 EFL port implementation with be more C++ oriented.
I created a meta bug for this task: https://bugs.webkit.org/show_bug.cgi?id=99696
I also wrote a first patch to show what the result looks like for the Ewk_Download_Job class. The patch also adds convenience macros to ewk_private.h to make pimpl idiom usage simpler and less error-prone: https://bugs.webkit.org/show_bug.cgi?id=99697
We hope you like the result as much as we do. If everyone agrees, Mikhail and I are planning to port other Ewk classes to this proposal and update the WebKit EFL coding style.
Is it really necessary to make this a Qt-like pimpl?
AFAIK Qt has this opaque d-pointer to keep ABI compatibility across releases. Since we are working on private code, I don't see need for this. Will just complexity to the code and extra pointers dereferences.
I think this point of view is really impressive. :) Given information on web, pimpl idioms is usually used when we hide information from the outer program coupled with shared library. This is because we don't have to or can't rebuild all outer applications when data structure need modification. As tmpsantos pointed, we are discussing about private functions.
Don't get me wrong, the refactoring is indeed necessary (but can be done differently). What I'm questioning here is the need of these macros and the "d->private" idiom. The change bellow is fine, removed C-ism from the code:
Yes, I want to avoid macro usage as much as we could since it is C-ism.
- uint64_t downloadId = ewk_download_job_id_get(ewkDownload); + uint64_t downloadId = ewkDownload->id();
But I would write Ewk_Download_Job::id() like this:
uint64_t Ewk_Download_Job::id() const { return m_downloadProxyId; }
Instead of:
uint64_t Ewk_Download_Job::id() const { return d->downloadProxy->downloadID(); }
Why not instead of creating a Ewk_Download_Job_Data you store the information you need inside Ewk_Download_Job and mark as private attributes?
Cheers, _______________________________________________ webkit-efl mailing list webkit-efl@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-efl
participants (6)
-
Christophe Dumez
-
Dumez, Christophe
-
Kangil Han
-
Kenneth Rohde Christiansen
-
Pozdnyakov, Mikhail
-
Thiago Marcos P. Santos