Hello All, Because a pre-review runs this days in collaborator tool I still want to provide some feedback on this library I was looking on long time ago. - What is your evaluation of the design? --------------------------------------------- This is the major problem of the library: It tries to do simple things overcomplicated. --------------------------------------------- Take as an example: struct Book : public pimpl<Book>::value_semantics { ... }; Is an attempt to do things in "fancy" way where it is not needed. struct Book { public: private: struct implementation; smart_ptr<implementation> pimpl_; }; Is much clearer. The proposed solution is overcomplicated hard to read and in general makes more troubles then helps, makes the code less readable by average programmers. ------------------------------------ shared_ptr as pointer model is Wrong ------------------------------------ You almost never want to have shared object semantics for pimplized object. I use pimpl ideom widely in my projects CppCMS, CppDB and even in Boost.Locale and I used shared semantics only once where it was explicitly shared. In rest of the cases I had found myself using following smart pointers: 1. copy_ptr - pointer the copies underlying object on its copy. i.e. something like copy_ptr(copy_ptr const &other) : p_(new Object(*other)) {} Copy the object on copy. 2. clone_ptr - pointer that clones underlying object on its copy. i.e. something like copy_ptr(copy_ptr const &other) : p_(other->clone()) {} 3. hold_ptr - pointer with same const-semantics as an object it points to T const &operator*() const; T &operator*() ; T const *operator->() const; T *operator->() ; They cover almost all use cases, so with addon of shared_ptr it covers ALL use cases. In any case current boost::scoped_ptr or std::auto_ptr is much more suitable smart pointer for implementing pimpl then Boost.pimpl. The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor. i.e. class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; }; No good. You need to add ~Foo(); So it would know to destroy Foo::Data correctly. But this can be easily solved on hold_ptr, copy_ptr and clone_ptr level same as it solved for shared_ptr and in reality even this not needed as it is better to add empty destructor in cpp. Spare Pointer use case ------------------------ Another important use case in the case when pimpl is used to handle ABI stability and in general the object has spare "pimpl" or d-pointer for future uses. Consider: struct book { public: book(); book(book const &); ~book(book const &); book const &operator=(book const &); std::string title() const; std::string author() const; void title(std::string const &); void author(std::string const &); private: std::string author_; std::string title_; struct data; copy_ptr<data> d; }; Now in book.cpp we define data as struct book::data {}; And we do not initialize d at all. It is fast and efficient object that does not use data at all. Now when we decide to add new member "isbn" without breaking ABI. in book.cpp we define in book.cpp struct book::data { std::string isbn_; } And now we initialized the d pointer in constructor. And add new members in book.h std::string isbn() const void isbn(std::String const &); And implement them std::string book::isbn() const { return d->isbn_; } void isbn(std::string const &s) { d->isbn_ = s; } This is very important use case the the author does not even relate to. - What is your evaluation of the implementation? Generally not looked into too deeply but it is does not follow boost standards: - Naming conventions - Documentation - Build So it is not even ready for the review (even thou it can be easily changed as it is a small library) - What is your evaluation of the documentation? 1. The documentation should be in HTML file not on some Internet web site. 2. Rationale part missing The documentation in not Boost-Ready - What is your evaluation of the potential usefulness of the library? Useless for 90% of pimpl use cases. - Did you try to use the library? With what compiler? Did you have any problems? No I hadn't - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? About 4-5 hours of studying - Are you knowledgeable about the problem domain? Yes, I work with pimpl-ized object on daily bases in many different approaches including implementing my own smart pointers for pimpl (clone_ptr, hold_ptr, copy_ptr) - ABI Stability - Compilation time reduction, - Decoupling of the interface from implementation details. Should The library be accepted in Boost: ======================================== No it should not. It is "a fancy" library that was written without a knowledge of the problem domain. It tries to solve problem in smart way making it overcomplicated. IMHO old std::auto_ptr is much more suitable for writing pimpl-objects then Boost.Pimple. For easy pimpl-ideom you do not need a fancy class but rather a set of pimpl-suitable smart pointers. Best Regards, Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
2011/5/26 Artyom Beilis <artyomtnk@yahoo.com>
Hello All,
Because a pre-review runs this days in collaborator tool I still want to provide some feedback on this library I was looking on long time ago.
- What is your evaluation of the design?
--------------------------------------------- This is the major problem of the library: It tries to do simple things overcomplicated. ---------------------------------------------
Take as an example:
struct Book : public pimpl<Book>::value_semantics { ... };
Is an attempt to do things in "fancy" way where it is not needed.
struct Book { public: private: struct implementation; smart_ptr<implementation> pimpl_; };
Is much clearer.
The proposed solution is overcomplicated hard to read and in general makes more troubles then helps, makes the code less readable by average programmers.
------------------------------------ shared_ptr as pointer model is Wrong ------------------------------------
You almost never want to have shared object semantics for pimplized object.
I use pimpl ideom widely in my projects CppCMS, CppDB and even in Boost.Locale and I used shared semantics only once where it was explicitly shared.
In rest of the cases I had found myself using following smart pointers:
1. copy_ptr - pointer the copies underlying object on its copy. i.e. something like
copy_ptr(copy_ptr const &other) : p_(new Object(*other)) {}
Copy the object on copy.
2. clone_ptr - pointer that clones underlying object on its copy. i.e. something like
copy_ptr(copy_ptr const &other) : p_(other->clone()) {}
3. hold_ptr - pointer with same const-semantics as an object it points to
T const &operator*() const; T &operator*() ; T const *operator->() const; T *operator->() ;
They cover almost all use cases, so with addon of shared_ptr it covers ALL use cases.
In any case current boost::scoped_ptr or std::auto_ptr is much more suitable smart pointer for implementing pimpl then Boost.pimpl.
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
But this can be easily solved on hold_ptr, copy_ptr and clone_ptr level same as it solved for shared_ptr and in reality even this not needed as it is better to add empty destructor in cpp.
Spare Pointer use case ------------------------
Another important use case in the case when pimpl is used to handle ABI stability and in general the object has spare "pimpl" or d-pointer for future uses.
Consider:
struct book { public: book(); book(book const &); ~book(book const &); book const &operator=(book const &); std::string title() const; std::string author() const; void title(std::string const &); void author(std::string const &); private: std::string author_; std::string title_; struct data; copy_ptr<data> d; };
Now in book.cpp we define data as struct book::data {};
And we do not initialize d at all. It is fast and efficient object that does not use data at all.
Now when we decide to add new member "isbn" without breaking ABI.
in book.cpp we define in book.cpp
struct book::data { std::string isbn_; }
And now we initialized the d pointer in constructor.
And add new members in book.h
std::string isbn() const void isbn(std::String const &);
And implement them
std::string book::isbn() const { return d->isbn_; } void isbn(std::string const &s) { d->isbn_ = s; }
This is very important use case the the author does not even relate to.
- What is your evaluation of the implementation?
Generally not looked into too deeply but it is does not follow boost standards:
- Naming conventions - Documentation - Build
So it is not even ready for the review (even thou it can be easily changed as it is a small library)
- What is your evaluation of the documentation?
1. The documentation should be in HTML file not on some Internet web site. 2. Rationale part missing
The documentation in not Boost-Ready
- What is your evaluation of the potential usefulness of the library?
Useless for 90% of pimpl use cases.
- Did you try to use the library? With what compiler? Did you have any problems?
No I hadn't
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
About 4-5 hours of studying
- Are you knowledgeable about the problem domain?
Yes, I work with pimpl-ized object on daily bases in many different approaches including implementing my own smart pointers for pimpl (clone_ptr, hold_ptr, copy_ptr)
- ABI Stability - Compilation time reduction, - Decoupling of the interface from implementation details.
Should The library be accepted in Boost: ========================================
No it should not. It is "a fancy" library that was written without a knowledge of the problem domain. It tries to solve problem in smart way making it overcomplicated.
IMHO old std::auto_ptr is much more suitable for writing pimpl-objects then Boost.Pimple.
For easy pimpl-ideom you do not need a fancy class but rather a set of pimpl-suitable smart pointers.
Best Regards,
Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
Hello, I totally agree with your review, Artym, except that I don't have knowledge about ABI changes to agree or disagree. I had a discussion with Ivan on this list about Boost.Pimpl some time ego, when he was probing interest, and my conclusion was similar to yours, but I had used different arguments. I won't bring my arguments, because I find your argumentation better and sufficient ;-) Anyway, I think boost lacks tools you called copy/clone_ptr, and hold_ptr. I wrote my own versions of those, and use them widely for Pimpl just like you say in your review. Would it make sense to propose them as an addition to smart_ptr maybe? I know there were some clone_ptr propositions in the past, so I never proposed my versions... But maybe in the context of Pimpl, they would be found useful? Regards Kris
Artyom Beilis wrote:
Should The library be accepted in Boost: ========================================
No it should not. It is "a fancy" library that was written without a knowledge of the problem domain. It tries to solve problem in smart way making it overcomplicated.
IMHO old std::auto_ptr is much more suitable for writing pimpl-objects then Boost.Pimple.
For easy pimpl-ideom you do not need a fancy class but rather a set of pimpl-suitable smart pointers.
I haven't reviewed the Pimpl library, but this comparison to "old std::auto_ptr" looks very strange to me. If I understood correctly, one of the reasons why "old std::auto_ptr" is deprecated in favor of "std::unique_ptr" is that "old std::auto_ptr" will silently break the default generated copy-constructor and assignment operator of a class containing an "old std::auto_ptr" as member. "std::unique_ptr" on the other hand will disable default generated copy-constructor and assignment operator and only default generate the "move" versions of these, so the user will at least realize that he has to manually provide correct versions of copy-constructor and assignment operator. Did I totally misunderstood you here, or was this comparison intended as a ironic remark, or is my understanding old "old std::auto_ptr" incorrect, or ...? Regards, Thomas
Thomas Klimpel wrote:
Artyom Beilis wrote:
Should The library be accepted in Boost: ========================================
No it should not. It is "a fancy" library that was written without a knowledge of the problem domain. It tries to solve problem in smart way making it overcomplicated.
IMHO old std::auto_ptr is much more suitable for writing pimpl-objects then Boost.Pimple.
For easy pimpl-ideom you do not need a fancy class but rather a set of pimpl-suitable smart pointers.
I haven't reviewed the Pimpl library, but this comparison to "old std::auto_ptr" looks very strange to me. If I understood correctly, one of the reasons why "old std::auto_ptr" is deprecated in favor of "std::unique_ptr" is that "old std::auto_ptr" will silently break the default generated copy- constructor and assignment operator of a class containing an "old std::auto_ptr" as member. "std::unique_ptr" on the other hand will disable default generated copy-constructor and assignment operator and only default generate the "move" versions of these, so the user will at least realize that he has to manually provide correct versions of copy-constructor and assignment operator.
It is these issues and the several manual steps required to implement the Pimpl Idiom that this library tries to address and simplify. It is totally clear that one can use shared_ptr, unique_ptr, etc., or even a raw pointer to implement the Pimpl Idiom. However, each such choice requires different member functions to effect proper copy and equality semantics, for example. Remembering to do everything and doing it correctly every time, for each case, is the problem. It may be that Artyom has used Pimpl so often that he never makes mistakes in its implementation for each use case. He may understand the issues completely and may always do the right thing. That doesn't mean that others will do as well. Libraries exist to capture design and implementation details so others can build upon that effort, even without fully understanding the issues. It may be that this library needs to be extended to support cloning, holding, and other use cases the author hadn't identified. That hardly justifies saying that the author does not have "knowledge of the problem domain." _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
From: "Stewart, Robert" <Robert.Stewart@sig.com>
Thomas Klimpel wrote:
Artyom Beilis wrote:
Should The library be accepted in Boost: ========================================
No it should not. It is "a fancy" library that was written without a knowledge of the problem domain. It tries to solve problem in smart way making it overcomplicated.
IMHO old std::auto_ptr is much more suitable for writing pimpl-objects then Boost.Pimple.
For easy pimpl-ideom you do not need a fancy class but rather a set of pimpl-suitable smart pointers.
I haven't reviewed the Pimpl library, but this comparison to "old std::auto_ptr" looks very strange to me. If I understood correctly, one of the reasons why "old std::auto_ptr" is deprecated in favor of "std::unique_ptr" is that "old std::auto_ptr" will silently break the default generated copy- constructor and assignment operator of a class containing an "old std::auto_ptr" as member. "std::unique_ptr" on the other hand will disable default generated copy-constructor and assignment operator and only default generate the "move" versions of these, so the user will at least realize that he has to manually provide correct versions of copy-constructor and assignment operator.
It is these issues and the several manual steps required to implement the Pimpl Idiom that this library tries to address and simplify.
Simplification does not imply that user must derive its own class from other class?! It does not simplifies things, it makes them more complicated. Consider following copy_ptr that both safe and simple. template<typename T> class copy_ptr { template<typename O> explicit copy_ptr(O *p) : ptr_(p), deleter_(deleter<O>),copy_(copy<O>) {} copy_ptr(copy_ptr const &other) : ptr_(0), deleter_(other.deleter_); copy_(other.copy_) { ptr_ = copy_(other.ptr_); } ~copy_ptr() { if(ptr_) deleter_(ptr_) } ... private: T *ptr_; void (*deleter_)(T *p); T *(*copy_)(T const *p); template<typename O> static void deleter(T *p) { delete static_cast<O *>(p); } template<typename O> static T *copy(T *p) { return new O(static_cast<O const &>(*p); } } It is safe for use by total newbie, but it does not require from a user to derive its own class from other one. The proposed solution is too complicated and overkill. It does not makes things simpler.
It is totally clear that one can use shared_ptr, unique_ptr, etc. or even a raw pointer to implement the Pimpl Idiom.
However, each such choice requires different member functions to effect proper copy and equality semantics,
With copy_ptr above no problem with copy semantics or other semantics. Also note Boost.Pimpl's equality semantics is wrong. You don't compare pointers to implementations but implementations themselves. So operator==(Book const &other) ( impl_ == other.impl_ /* as pointer */ } Is wrong, you need to compare *impl_ == *other.impl_ And it is not possible to do in header file as actual implementation required.
It may be that Artyom has used Pimpl so often that he never makes mistakes in its implementation for each use case. He may understand the issues completely and may always do the right thing. That doesn't mean that others will do as well. Libraries exist to capture design and implementation details so others can build upon that effort, even without fully understanding the issues.
That is why I had created a set of smart pointers for pimpl. It has nothing to do with making mistakes. That is why copy_ptr solves the problem and hold_ptr (that is just non-copyable copy_ptr) solves the problem as well. They are safe but also simple. Simplicity is the key. Derivation from other class is not simplicity. Writing template<> class pimpl<foo>::implementation Over foo::implementation Is not simplicity. Especially when it limits your to very specific type when you may hide some other existing complex type that you may just not show.
It may be that this library needs to be extended to support cloning, holding, and other use cases the author hadn't identified.
That hardly justifies saying that the author does not have "knowledge of the problem domain."
I'm sorry if it seems rude. However, some design issues in the library design make me feel that too many important uses cases are just not covered, that's it. For example, consider something like xml_document { public: ... private: smart_ptr<xmlDocPtr> pimpl_; } Very popular type use case when one class if a facade of other complicated one or one that is does not exposed to user. Can't be done with Boost.Pimpl as pointer to implementation is restricted to specific type. The problem is that the library solves only one particular use case and does not see too many other very popular use cases existing in the real software. Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
From: Thomas Klimpel <Thomas.Klimpel@synopsys.com>
Did I totally misunderstood you here, or was this comparison intended as a ironic remark, or is my understanding old "old std::auto_ptr" incorrect, or ...?
It was indeed half ironic and half not. For non-copyable classes std::auto_ptr IS better then Boost.Pimpl as Boost.Pimpl does not even provide support of non-copyable implementations. So if the class in non-copyable std::auto_ptr is INDEED better, because in such case auto_ptr == scoped_ptr However it is also irony as IMHO it is easier to workaround auto_ptr copy/assign issue manually then use Boost.Pimpl that requires that my class would be derived from some other fancy (and almost useless class) writing a verbose code. Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/ P.S.: Shame that auto_ptr was deprecated...
Am 26.05.2011 11:22, schrieb Artyom Beilis:
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329): "In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component." which applies to most standard library types.
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 11:22, schrieb Artyom Beilis:
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329):
"In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component."
which applies to most standard library types.
When you actually implement Foo::~Foo() {} in cpp file the type of the data object is complete so the destructor is installed correctly. It is undefined if you have inline destructor that does not have defined "struct data" but it is defined for non-inline destructor in cpp that has fully defined "struct data" So there is nothing wrong there neither in reality nor by the standard. Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Am 26.05.2011 18:28, schrieb Artyom Beilis:
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 11:22, schrieb Artyom Beilis:
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329):
"In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component."
which applies to most standard library types.
When you actually implement Foo::~Foo() {} in cpp file the type of the data object is complete so the destructor is installed correctly.
That's not correct. std::auto_ptr<data> is an instantiation which uses an incomplete type as template argument. Providing an non-inline destructor doesn't magically make the data struct used in your example a complete type.
It is undefined if you have inline destructor that does not have defined "struct data" but it is defined for non-inline destructor in cpp that has fully defined "struct data"
So there is nothing wrong there neither in reality nor by the standard.
Your code produces undefined behavior and thus is not portable. See above.
----- Original Message ----
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 18:28, schrieb Artyom Beilis:
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 11:22, schrieb Artyom Beilis:
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329):
"In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component."
which applies to most standard library types.
When you actually implement Foo::~Foo() {} in cpp file the type of the data object is complete so the destructor is installed correctly.
That's not correct. std::auto_ptr<data> is an instantiation which uses an incomplete type as template argument. Providing an non-inline destructor doesn't magically make the data struct used in your example a complete type.
The quote you provide is too general about the library requirements. Particularly, looking into auto_ptr. (20.4.5.1 C++/2003 standard) ~auto_ptr() throw(); Requires: The expression delete get() is well formed. Effects: delete get(). The expression delete get() is well formed when ~auto_ptr() is called in the destructor of Foo::~Foo() defined in cpp file. So it fills auto_ptr requirements. No problems I can see also I'm not aware of any compiler that does not handle this correctly (so far Intel, GCC, MSVC, SunStudio, HP's ACC, DECXX) Artyom
Artyom Beilis wrote:
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 18:28, schrieb Artyom Beilis:
From: Sergiu Dotenco <sergiu.dotenco@gmail.com> Am 26.05.2011 11:22, schrieb Artyom Beilis:
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329):
"In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component."
which applies to most standard library types.
When you actually implement Foo::~Foo() {} in cpp file the type of the data object is complete so the destructor is installed correctly.
Not relevant. Foo is an incomplete type and it is used as a template argument for std::auto_ptr in the header.
That's not correct. std::auto_ptr<data> is an instantiation which uses an incomplete type as template argument. Providing an non-inline destructor doesn't magically make the data struct used in your example a complete type.
The quote you provide is too general about the library requirements.
No. It's right on point.
Particularly, looking into auto_ptr. (20.4.5.1 C++/2003 standard)
~auto_ptr() throw(); Requires: The expression delete get() is well formed. Effects: delete get().
The expression delete get() is well formed when ~auto_ptr() is called in the destructor of Foo::~Foo() defined in cpp file.
That doesn't negate the point about the template argument.
So it fills auto_ptr requirements.
Nope.
No problems I can see also I'm not aware of any compiler that does not handle this correctly (so far Intel, GCC, MSVC, SunStudio, HP's ACC, DECXX)
That doesn't mean the code is conforming. You can choose to rely on UB in your code, but Boost strives to do better. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
Am 26.05.2011 21:23, schrieb Artyom Beilis:
----- Original Message ----
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 18:28, schrieb Artyom Beilis:
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 11:22, schrieb Artyom Beilis:
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329):
"In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component."
which applies to most standard library types.
When you actually implement Foo::~Foo() {} in cpp file the type of the data object is complete so the destructor is installed correctly.
That's not correct. std::auto_ptr<data> is an instantiation which uses an incomplete type as template argument. Providing an non-inline destructor doesn't magically make the data struct used in your example a complete type.
The quote you provide is too general about the library requirements.
That doesn't make the quoted requirement less valid.
Particularly, looking into auto_ptr. (20.4.5.1 C++/2003 standard)
~auto_ptr() throw(); Requires: The expression delete get() is well formed. Effects: delete get().
The expression delete get() is well formed when ~auto_ptr() is called in the destructor of Foo::~Foo() defined in cpp file.
So it fills auto_ptr requirements.
I don't see how is that relevant. The non-inline destructor you referred to previously is needed for correct object deallocation (ISO/IEC 14882:2003, §5.3.5, p. 83, par. 5). Obviously, templates are not instantiated in the destructor, are they? The std::auto_ptr<> template used in your example is instantiated right below the struct data; line at which point data is still an incomplete type. Hence the effects are undefined and the code is not portable.
No problems I can see also I'm not aware of any compiler that does not handle this correctly (so far Intel, GCC, MSVC, SunStudio, HP's ACC, DECXX)
Well, that's not the point. Consider what happens if a library vendor decides to perform a (fully legitimate) check, as in template<class T> class auto_ptr { // ... static_assert(__is_complete_type(T), "incomplete type used"); }; If this happens, your pimpl version will no longer work.
2011/5/26 Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 21:23, schrieb Artyom Beilis:
----- Original Message ----
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 18:28, schrieb Artyom Beilis:
From: Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 11:22, schrieb Artyom Beilis:
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
i.e.
class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; };
No good. You need to add
~Foo();
So it would know to destroy Foo::Data correctly.
You can't use std::auto_ptr for pimpl implementation in portable code. Doing so is undefined behavior. To quote ISO/IEC 14882:2003 (§17.4.3.6, p. 329):
"In particular, the effects are undefined in the following cases: [...] — if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component."
which applies to most standard library types.
When you actually implement Foo::~Foo() {} in cpp file the type of the data object is complete so the destructor is installed correctly.
That's not correct. std::auto_ptr<data> is an instantiation which uses an incomplete type as template argument. Providing an non-inline destructor doesn't magically make the data struct used in your example a complete type.
The quote you provide is too general about the library requirements.
That doesn't make the quoted requirement less valid.
Particularly, looking into auto_ptr. (20.4.5.1 C++/2003 standard)
~auto_ptr() throw(); Requires: The expression delete get() is well formed. Effects: delete get().
The expression delete get() is well formed when ~auto_ptr() is called in the destructor of Foo::~Foo() defined in cpp file.
So it fills auto_ptr requirements.
I don't see how is that relevant.
The non-inline destructor you referred to previously is needed for correct object deallocation (ISO/IEC 14882:2003, §5.3.5, p. 83, par. 5). Obviously, templates are not instantiated in the destructor, are they? The std::auto_ptr<> template used in your example is instantiated right below the struct data; line at which point data is still an incomplete type. Hence the effects are undefined and the code is not portable.
No problems I can see also I'm not aware of any compiler that does not handle this correctly (so far Intel, GCC, MSVC, SunStudio, HP's ACC, DECXX)
Well, that's not the point. Consider what happens if a library vendor decides to perform a (fully legitimate) check, as in
template<class T> class auto_ptr { // ... static_assert(__is_complete_type(T), "incomplete type used"); };
If this happens, your pimpl version will no longer work.
Looks like auto_ptr really isn't suitable for pimpl. And auto_ptr has many more draw-backs, so it is deprecated. But getting back to the mini-review, suppose we use scoped_ptr instead of auto_ptr. I think Artyoms arguments still stand. And then you can't forget to make a non-inline destructor, because scoped_ptr uses checked_delete. Regards Kris
Am 26.05.2011 23:30, schrieb Krzysztof Czainski:
Looks like auto_ptr really isn't suitable for pimpl. And auto_ptr has many more draw-backs, so it is deprecated.
But getting back to the mini-review, suppose we use scoped_ptr instead of auto_ptr. I think Artyoms arguments still stand. And then you can't forget to make a non-inline destructor, because scoped_ptr uses checked_delete.
I certainly prefer using a specialized and well-tested library requiring just a couple of lines over manually implementing such an essential idiom by introducing boilerplate-code. Not mentioning the case when value semantics are wanted which scoped_ptr doesn't support.
2011/5/27 Sergiu Dotenco <sergiu.dotenco@gmail.com>
Am 26.05.2011 23:30, schrieb Krzysztof Czainski:
Looks like auto_ptr really isn't suitable for pimpl. And auto_ptr has many more draw-backs, so it is deprecated.
But getting back to the mini-review, suppose we use scoped_ptr instead of auto_ptr. I think Artyoms arguments still stand. And then you can't forget to make a non-inline destructor, because scoped_ptr uses checked_delete.
I certainly prefer using a specialized and well-tested library requiring just a couple of lines over manually implementing such an essential idiom by introducing boilerplate-code. Not mentioning the case when value semantics are wanted which scoped_ptr doesn't support.
Yes, I agree that scoped_ptr does not suffice, but I take Artyoms point about prefering a simple scoped_ptr (or as he said, auto_ptr) over the proposed Pimpl. Personally I think the proposed Pimpl library tries to solve too many things at once. Individual use cases might want those, not care about them, or specifically not want them. For example some class may want: relational operators; the proper deployment of the incomplete-type management technique (for the destructors to always work correctly). That class might not care about: support for in-place and replace boost::serialization; support for value and pointer semantics (and others if needed policies); support for the Bridge pattern; support for Non-Virtual Interface Idiom; And Pimpl providing the following: safebool operator; pimpl::null; can be an unwanted side-effect, and may therefore lead to a decision not to use the proposed Pimpl in this case. Many of the features addressed by Pimpl can be provided by separate tools, instead of packing all of them to Pimpl. That is why I'd be happy to see in boost: clone/copy_ptr, as well as a smart pointer with value semantics (I believe Artyom called it hold_ptr, and I like to call it value_ptr). These would also provide the "proper deployment of the incomplete-type management technique (for the destructors to always work correctly)". Relational operators can be easily provided using Boost.Operators. A subset of the operators would have to be implemented by hand, but putting them into Pimpl means every user of Pimpl get's them. And if that user doesn't want his class to have these operators? Will tools like the new traits for operators (has_operator_less etc.) detect them, when they shouldn't? As for safebool operator, I'd propose adding something like a boolable class to the operators library. It could require operator!, and implement a safebool operator in terms of op!. To conclude, I agree, that the problems addressed by the proposed Pimpl need solutions, but I suggest providing separate tools addressing the problems instead of Pimpl addressing all of these. With the proposed Pimpl, I would often choose not to use it and to implement pimpl by hand instead, just because Pimpl provides provides too many features. Regards Kris
Krzysztof Czainski <1czajnik <at> gmail.com> writes: ... Personally I think the proposed Pimpl library tries to solve too many things at once.
On the bigger scale it tries just *one* thing -- to implement the Pimpl idiom as it is understood by me (which might well be misunderstanding). On the smaller scale it tries to do *two* things -- 1) the separation of interface and implementation and 2) implementation hiding. It is because that is how I understand the properties/qualities/characteristics of the Pimpl idiom. What exactly more does it do? Is it the relational ops you do not like? Then, say, shared_ptr has them and one needs them if a Pimpl-based class is to be used, say, in std::containers. Is it null() that keeps you awake at night? Then, many classes (like shared_ptr again) have it... maybe not in as explicit form but still. Is it the support for the Bridge pattern and Non-Virtual Interface Idiom that get you worried? Then, ignore that -- that's not a stone in your shoe, is it?
Individual use cases might want those, not care about them, or specifically not want them.
So, if I follow your line of reasoning correctly, then I should discard boost::regex as it provides support for Perl Regular Expression Syntax POSIX Extended Regular Expression Syntax POSIX Basic Regular Expression Syntax when I only "want/care for" the Perl syntax? Do you pay for that additional functionality? Is it heavy to carry? Is it getting in your way somehow? Does it result in unexpected behavior? Have you noticed that *any* library actually provides *more* than *you* usually need because such library might be written with wider audience in mind? Jeez, I do not believe we are even discussing it!
... And Pimpl providing the following: safebool operator; pimpl::null; can be an unwanted side-effect, and may therefore lead to a decision not to use the proposed Pimpl in this case.
Are you just pondering or have something concrete in mind? If that is the latter, then bring it on. We'll look at it and fix it... or I'll say, shit, you are right, bad-bad design... and fix it again. Otherwise, all these "can" and "may" are just noise.
Many of the features addressed by Pimpl can be provided by separate tools, instead of packing all of them to Pimpl.
Yeah, and next time you go to buy, say, a car, please to not accept a car as a whole but insist on wheels, radio, etc. separately. I am sure you'll enjoy the ride.
... To conclude, I agree, that the problems addressed by the proposed Pimpl need solutions, but I suggest providing separate tools addressing the problems instead of Pimpl addressing all of these.
So, would you please come forward and suggest something tangible addressing those problems. I am sure you are prepared that that features that you care about might not be so dear to somebody else's heart. V.
On 5/26/11 8:06 PM, Vladimir Batov wrote:
Krzysztof Czainski<1czajnik<at> gmail.com> writes: ... Personally I think the proposed Pimpl library tries to solve too many things at once. On the bigger scale it tries just *one* thing -- to implement the Pimpl idiom as it is understood by me (which might well be misunderstanding). On the smaller scale it tries to do *two* things -- 1) the separation of interface and implementation and 2) implementation hiding. It is because that is how I understand the properties/qualities/characteristics of the Pimpl idiom. What exactly more does it do? Is it the relational ops you do not like? Then, say, shared_ptr has them and one needs them if a Pimpl-based class is to be used, say, in std::containers. Is it null() that keeps you awake at night? Then, many classes (like shared_ptr again) have it... maybe not in as explicit form but still. Is it the support for the Bridge pattern and Non-Virtual Interface Idiom that get you worried? Then, ignore that -- that's not a stone in your shoe, is it?
Vladimir, your frustration is entirely understandably (and indeed, I would feel the same way if I were in your position), but I would like to respectfully suggest that right after criticizing the tone of others for being harsh it is generally a good idea to be extra careful to police your own tone. Having said that, I do agree with your thesis but would restate in the following way: even if it also made sense to have the parts of pimpl available as separate tools, it is nonetheless would be a great boon to ordinary users to have a "one stop shop" where they could just get everything that they need and not have to worry about it, which is exactly what pimpl attempts to provide. So even if all the parts were available separately, I think that we would want something like pimpl to combine them all for the common case.
Many of the features addressed by Pimpl can be provided by separate tools, instead of packing all of them to Pimpl. Yeah, and next time you go to buy, say, a car, please to not accept a car as a whole but insist on wheels, radio, etc. separately. I am sure you'll enjoy the ride.
How about the following compromise? Why not break pimpl up into a number of subclasses that share a common base virtual class; then the user only wants some of the features, they select whatever custom features that they want by choosing those particular base classes. Or, if the user isn't experienced enough to know what they want, they can just inherit from the a default choice will be provided that contains everything. Furthermore, this provides an infrastructure basis that can be further extended with other features both by the pimpl library and also by users in the future. I would implement this by having all of the "feature" classes have trivial constructors. As I understand it, virtual constructors are called immediately from the most derived class so the user will just need call the pimpl base class from their own code to initialize the member data and all of the other calls to this virtual base constructor will be ignored. All of the extra checks to see whether the virtual base class had been constructed would add some overhead to the construction cost, but hopefully not too much. Personally I think that this provides a solution that addresses both concerns and provides additional potential for the library to be useful now and in the future. Thoughts? Cheers, Greg
Gregory Crosswhite <gcross <at> phys.washington.edu> writes: ... Vladimir, your frustration is entirely understandably (and indeed, I would feel the same way if I were in your position), but I would like to respectfully suggest that right after criticizing the tone of others for being harsh it is generally a good idea to be extra careful to police your own tone.
Yes, I know. Apologies. Truly. Turning into an angry old man. Not pretty.
... How about the following compromise?
Why not break pimpl up into a number of subclasses that share a common base virtual class; then the user only wants some of the features, they select whatever custom features that they want by choosing those particular base classes. Or, if the user isn't experienced enough to know what they want, they can just inherit from the a default choice will be provided that contains everything. Furthermore, this provides an infrastructure basis that can be further extended with other features both by the pimpl library and also by users in the future.
I would implement this by having all of the "feature" classes have trivial constructors. As I understand it, virtual constructors are called immediately from the most derived class so the user will just need call the pimpl base class from their own code to initialize the member data and all of the other calls to this virtual base constructor will be ignored. All of the extra checks to see whether the virtual base class had been constructed would add some overhead to the construction cost, but hopefully not too much.
My first reaction is "I do not know" -- in its true meaning. I am somewhat slow, so I have to sleep of that idea to better understand what it brings and what the price/benefits might be. I am probably Pimpl-indoctrinated and I am certainly this implementation-biased, so to me OTOH it does not offer really that much to dwell on or to try to split into smaller parts -- the code is only about 100 lines (comments excluded). I guess, we are talking beyond taking safebool and value_semantics_ptr out. To understand better I'll have to see concrete examples/use-cases and how splitting Pimpl (or re-doing it) would help help to solve some concrete issues and/or how the existing pimpl fails to address those. Maybe, Krzysztof and Artyom would kindly provide some code snippets for an old man like me to chew on. Again, my sincere apologies, I hope no hard feelings. V.
2011/5/27 Vladimir Batov <vb.mail.247@gmail.com>
Gregory Crosswhite <gcross <at> phys.washington.edu> writes: ... Vladimir, your frustration is entirely understandably (and indeed, I would feel the same way if I were in your position), but I would like to respectfully suggest that right after criticizing the tone of others for being harsh it is generally a good idea to be extra careful to police your own tone.
Yes, I know. Apologies. Truly. Turning into an angry old man. Not pretty.
If my tone was found impolite too, my apologies - that was not intended at all. I just wanted to express my thoughts about this library, which in this case were negative ones, but I assure you this isn't always the case ;-) Regards, Kris
"Krzysztof Czainski" wrote ... If my tone was found impolite too, my apologies - that was not intended at all. I just wanted to express my thoughts about this library, which in this case were negative ones, but I assure you this isn't always the case ;-)
Look, Kris, you have nothing to apologise for. I took my frustration out on an innocent passer-by... As it often happens in our not-so-fair life. So, apologies are all mine. Getting back to your emails about Pimpl, I firmly believe that critisism is healthy and in my experience it always results in better software. I always strive to address whatever suggestions/criticism/feedback I can get. What I found somewhat frustrating with your comments was that I could not address/fix things which you seemingly was not happy about. Say, you say "Pimpl library tries to solve too many things at once". It's the critisism which I as the developer want to address... but cannot. What things exactly you think do not belong in Pimpl? What am I supposed to do to make you happy? As it is, it's a one-sided show -- you criticise and I just sit there as a "whipping boy" unable to do anything about it -- not to fix it not to defend it. Not fun. Or you say "Individual use cases might want those, not care about them, or specifically not want them". My interpretation is like "it can do this, I do not care for this, so it's bad". In such a setting how a library developer is to satisty more than one customer if customer A says I do not care for the features needed by customer B and customer B is saying the opposite. If on the other hand you say I do not think safebool() belongs with Pimpl, then we can talk about it. In the end I might jump sides and admit my original design was wrong or I might have certain use-cases which you do "not care about" but I need to support to keep another customer happy. In the end we might agree to disagree but that'd be after a productive two-way discussion. I hope you see my point. Best, V.
2011/5/27 Vladimir Batov <vb.mail.247@gmail.com>
Krzysztof Czainski <1czajnik <at> gmail.com> writes: ... Personally I think the proposed Pimpl library tries to solve too many things at once.
On the bigger scale it tries just *one* thing -- to implement the Pimpl idiom as it is understood by me (which might well be misunderstanding).
I take your point here.
On the smaller scale it tries to do *two* things -- 1) the separation of interface and implementation and 2) implementation hiding. It is because that is how I understand the properties/qualities/characteristics of the Pimpl idiom. What exactly more does it do? Is it the relational ops you do not like? Then, say, shared_ptr has them and one needs them if a Pimpl-based class is to be used, say, in std::containers. Is it null() that keeps you awake at night? Then, many classes (like shared_ptr again) have it... maybe not in as explicit form but still. Is it the support for the Bridge pattern and Non-Virtual Interface Idiom that get you worried? Then, ignore that -- that's not a stone in your shoe, is it?
Suppose i write a class: class Gadget { public: Gadget(); int fun(); private: int x; }; Now later I decide, that it would be nice to use ExpenciveGadget in the implementation of my Gadget. But it's expensive, so I don't want my users (people who include Gadget.h) to pay for it (have it inculded together). So the first thin I think is, hay, Pimpl ;-) option a) (assuming we have a clone_ptr) class Gadget { public: Gadget(); int full(); private: struct Impl; clone_ptr<Impl> impl_; // now I moved int x into impl_ too, but it's irrelevant right now }; option b) class Gadget : public pimpl<Gadget>::value_semantics { public: Gadget(); int full(); }; Now the question is what else, besides my pointer to implementation, did I inherit form pimpl<Gadget>::value_semantics? And the answer is some more interface. operator<, operator safebool, null(), etc. I didn't want to extend my interface, but I got it for free. And all I'm saying is it can be bad even though usually it doesn't matter. Why bad? 1) Tyops: Gadget a; Madget b; // suppose Gadget is the above, and can't be null, but Madget can be null if ( a.null() /*oops, I ment a.full()*/ ) ... or if ( a /* oops, I ment b */ ) ... 2) template < class T > void abc( typename enable_if< has_operator_less_then<T> >::type* = 0 ) {...} template < class T > void abc( typename disable_if< has_operator_less_then<T> >::type* = 0 ) {...} Gadget g; abc(g); // now I want the second version of abc.. which gets called? Now you say shared_ptr has these operators, and what if I don't need them? Well, I don't inherit from shared_ptr when I decide to use it in my implementation. I make it a member, and the operators don't get exposed outside. 3) Don't pay for what you don't need. I just want to hide my implementation, while I get the unnecessary operators< etc. Ok, I take your point about regex below, that this argument is weak, but I think the two arguments above sitll stand.
Individual use cases might want those, not care about them, or specifically not want them.
So, if I follow your line of reasoning correctly, then I should discard boost::regex as it provides support for
Perl Regular Expression Syntax POSIX Extended Regular Expression Syntax POSIX Basic Regular Expression Syntax
when I only "want/care for" the Perl syntax? Do you pay for that additional functionality? Is it heavy to carry? Is it getting in your way somehow? Does it result in unexpected behavior? Have you noticed that *any* library actually provides *more* than *you* usually need because such library might be written with wider audience in mind? Jeez, I do not believe we are even discussing it!
... And Pimpl providing the following: safebool operator; pimpl::null; can be an unwanted side-effect, and may therefore lead to a decision not to use the proposed Pimpl in this case.
Are you just pondering or have something concrete in mind? If that is the latter, then bring it on. We'll look at it and fix it... or I'll say, shit, you are right, bad-bad design... and fix it again. Otherwise, all these "can" and "may" are just noise.
Many of the features addressed by Pimpl can be provided by separate tools, instead of packing all of them to Pimpl.
Yeah, and next time you go to buy, say, a car, please to not accept a car as a whole but insist on wheels, radio, etc. separately. I am sure you'll enjoy the ride.
... To conclude, I agree, that the problems addressed by the proposed Pimpl need solutions, but I suggest providing separate tools addressing the problems instead of Pimpl addressing all of these.
So, would you please come forward and suggest something tangible addressing those problems. I am sure you are prepared that that features that you care about might not be so dear to somebody else's heart.
V.
I hope you'll judge it with no hard feelings, Vladimir ;-) Regards Kris
Krzysztof Czainski wrote:
2011/5/27 Vladimir Batov <vb.mail.247@gmail.com>
Krzysztof Czainski <1czajnik <at> gmail.com> writes:
Personally I think the proposed Pimpl library tries to solve too many things at once.
On the bigger scale it tries just *one* thing -- to implement the Pimpl idiom as it is understood by me (which might well be misunderstanding).
Yes and no. The Pimpl Idiom is *only* about separating the interface and implementation. How that's done is the issue. The effect it has on the interface is also an issue. What the Pimpl library does is modify the interface of the class with a hidden implementation. Helping to implement copying and assignment is useful, provided the class is supposed to support those operations. The rest seems questionable, as I've discussed variously in the Code Collaborator comments. For me, the bottom line is that a UDT, with or without using Pimpl should look the same, in terms of interface, to the user of that type. Pimpl's presence should be just an implementation detail. That doesn't mean it cannot be a base class, just that its presence needs to be unobtrusive to the user of classes using the library. Given no effect on the interface of a class using Pimpl, where Pimpl can offer real value is in providing predefined policies for how to manage the implementation object and its lifetime. It could include policies to support things like SBO or cloning (as Artyom has noted).
What exactly more does it do? Is it the relational ops you do not like? Then, say, shared_ptr has them and one needs them if a Pimpl-based class is to be used, say, in std::containers. Is it null() that keeps you awake at night? Then, many classes (like shared_ptr again) have it... maybe not in as explicit form but still.
The problem isn't so much that pimpl provide those features, but that it adds them to the interface class because of inheritance. One doesn't derive from shared_ptr, for example. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
Stewart, Robert <Robert.Stewart <at> sig.com> writes:
Yes and no. The Pimpl Idiom is *only* about separating the interface and implementation.
... and hiding the implementation details. See, say, http://www.gotw.ca/gotw/024.htm, http://www.gotw.ca/publications/mill05.htm.
What the Pimpl library does is modify the interface of the class with a hidden implementation.
If that exposure is not desired, there is private inheritance.
Helping to implement copying and assignment is useful, provided the class is supposed to support those operations.
Pimpl does *not* provide copying and/or assignment. It delegates these "requests" to the policy. For shared_ptr-based Pimpls copying and assignment are done by shared_ptr. For value-semantics-based Pimpls copying and assignment are only available if the underlying implementation supports that. No surprises.
The rest seems questionable, as I've discussed variously in the Code Collaborator comments.
The rest are safebool, null, swap, reset. swap, reset again delegate to the policy classes. safebool, null are IMO idiomatic to the Pimpl concept as it is idiomatic for raw pointers.
For me, the bottom line is that a UDT, with or without using Pimpl should look the same, in terms of interface, to the user of that type.
It does. The implementer controls what is implemented/visible what what is not.
Pimpl's presence should be just an implementation detail.
Disagree. Pimpl is a programing concept and it brings relevant features/behavior. Like if I decide to pass args by pointers, I get respective pointer-related "features" (good or bad depends on one's view).
That doesn't mean it cannot be a base class, just that its presence needs to be unobtrusive to the user of classes using the library.
I am certainly biased, but I've never found Pimpl involvement being obtrusive.
Given no effect on the interface of a class using Pimpl, where Pimpl can offer real value is in providing predefined policies for how to manage the implementation object and its lifetime. It could include policies to support things like SBO or cloning (as Artyom has noted).
Sure. If Pimpl accepted/sticks around, it's reasonable to expect other policies (likely by other people) to emerge. I am not obviously planning on spending my time adding some other policies of no value to me without any guarantees my effort and time won't be wasted.
The problem isn't so much that pimpl provide those features, but that it adds them to the interface class because of inheritance.
It's up to the implementer to make use of those provided features or deny them.
One doesn't derive from shared_ptr, for example.
One does derive from boost::noncopyable, for example. V.
Hi, I just wanted to add an example... I think Pimpl should have the following property: a) struct X { int f(); private: int a_; }; b) struct X : public pimpl<X>::value_semantics { int f(); }; The user of class X should not be able to tell the difference between a) and b). X x; Now in this case the user can tell the difference. In b) he can do x.null(), but in a) he can't. So Pimpl changes the interface. And that's what I want to avoid. Pimpl or no Pimpl, the interface of X should stay the same. Regards Kris
Krzysztof Czainski <1czajnik <at> gmail.com> writes:
I just wanted to add an example... I think Pimpl should have the following property:
a) struct X { int f(); private: int a_; };
b) struct X : public pimpl<X>::value_semantics { int f(); };
The user of class X should not be able to tell the difference between a) and b).
X x;
Now in this case the user can tell the difference. In b) he can do x.null(), but in a) he can't. So Pimpl changes the interface. And that's what I want to avoid. Pimpl or no Pimpl, the interface of X should stay the same.
1) I firmly believe that the insistence on no difference between a) an b) is misguided. You are applying a programming pattern/idiom and you reap the benefits of that applications. 2) That said I do see your desire to deny x.null() to the user. I'd be interested to know why you are not noticing/satisfied with the obvious solution struct X : pimpl<X>::value_semantics { ...} V.
On 5/28/11 2:55 PM, Vladimir Batov wrote:
>> Krzysztof Czainski<1czajnik<at> gmail.com> writes:
>>
>> I just wanted to add an example...
>> I think Pimpl should have the following property:
>>
>> a)
>> struct X
>> {
>> int f();
>> private:
>> int a_;
>> };
>>
>> b)
>> struct X : public pimpl<X>::value_semantics
>> {
>> int f();
>> };
>>
>> The user of class X should not be able to tell the difference between a) and
>> b).
>>
>> X x;
>>
>> Now in this case the user can tell the difference. In b) he can do x.null(),
>> but in a) he can't. So Pimpl changes the interface. And that's what I want
>> to avoid. Pimpl or no Pimpl, the interface of X should stay the same.
> 1) I firmly believe that the insistence on no difference between a) an b) is
> misguided. You are applying a programming pattern/idiom and you reap the
> benefits of that applications.
>
> 2) That said I do see your desire to deny x.null() to the user. I'd be
> interested to know why you are not noticing/satisfied with the obvious solution
>
> struct X : pimpl<X>::value_semantics { ...}
>
Again, I don't see why we can't have it both ways by letting the user
mix and match what operations they want. Use virtual inheritance so
that the user can call the pimpl base constructor directly, and then use
either multiple inheritance or base class daisy-chaining to let the user
select whatever operations they want. This also creates an
infrastructure for us to add new operations such as operator[],
operator(), user-defined operators* and operator->, etc., which can
additional be mixed in to the interface.
Cheers,
Greg
Vladimir Batov wrote:
Stewart, Robert <Robert.Stewart <at> sig.com> writes:
Yes and no. The Pimpl Idiom is *only* about separating the interface and implementation.
... and hiding the implementation details. See, say, http://www.gotw.ca/gotw/024.htm, http://www.gotw.ca/publications/mill05.htm.
Separating the implementation from the interface is for the purpose of hiding the implementation. I think I covered it just fine.
Helping to implement copying and assignment is useful, provided the class is supposed to support those operations.
shared_ptr-based Pimpls copying and assignment are done by shared_ptr.
That tells me that Pimpl provides copying and assignment for the derivate.
For value-semantics-based Pimpls copying and assignment are only available if the underlying implementation supports that.
That is good.
The rest seems questionable, as I've discussed variously in the Code Collaborator comments.
The rest are
safebool, null, swap, reset.
Don't forget dereferencing and member selection.
swap, reset again delegate to the policy classes.
reset(), as I've noted, exposes the need to allocate memory using new to code that otherwise doesn't need it. The overload taking a deleter is a helpful addition, however.
safebool, null are IMO idiomatic to the Pimpl concept as it is idiomatic for raw pointers.
Those are idiomatic only to the implementation of the derivate (Book), not to clients of the derivate.
For me, the bottom line is that a UDT, with or without using Pimpl should look the same, in terms of interface, to the user of that type.
It does. The implementer controls what is implemented/visible what what is not.
As you've noted, this is only by virtue of using private inheritance. I still don't think it is correct behavior for a library implementing the Pimpl Idiom to modify the interface class' (Book's) interface. Consider a raw pointer Pimpl design: class Book { // state }; becomes: class Book { struct impl; impl pimpl_; }; struct Book::impl { // state }; Adding a pimpl did nothing to Book's interface. Why then should Book's interface chance when using your library? That seems to be an extension of the idiom into the territory of the Bridge Pattern or something else entirely.
Pimpl's presence should be just an implementation detail.
Disagree. Pimpl is a programing concept and it brings relevant features/behavior. Like if I decide to pass args by pointers, I get respective pointer-related "features" (good or bad depends on one's view).
Where has this been incorporated into "The Pimpl Idiom" in the literature (besides your article)? Have I missed something?
That doesn't mean it cannot be a base class, just that its presence needs to be unobtrusive to the user of classes using the library.
I am certainly biased, but I've never found Pimpl involvement being obtrusive.
Modifying the interface is the part I was referring to. You've said that copying and assignment are only supported (for value semantics, anyway) if the derivate provides them. That's a good approach. If everything is of that sort, then all's well, but that's not what I see.
One doesn't derive from shared_ptr, for example.
One does derive from boost::noncopyable, for example.
You were suggesting that shared_ptr provides pointer semantics, so it was reasonable that pimpl do so. My point was that one doesn't derive from shared_ptr, so the pointer semantics were relegated to shared_ptr data members rather than to the class itself. Therefore, you couldn't justify adding pointer semantics using shared_ptr as an exemplar. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Fri, May 27, 2011 at 9:40 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Given no effect on the interface of a class using Pimpl, where Pimpl can offer real value is in providing predefined policies for how to manage the implementation object and its lifetime. It could include policies to support things like SBO or cloning (as Artyom has noted).
I have/had a SBO "DeplorablePimpl" template (see Herb Sutter's http://www.gotw.ca/gotw/028.htm). Basically class X { DeplorableImpl<42, XImpl> pimpl; }; It takes care of most/all of Sutter's concerns. For example, it checks at compile time (in the cpp file) that sizeof(XImpl) == 42, (or <= 42, depending on policies) and forces correct constructors, etc. It does mean that X needs a constructor and destructor to be defined in the cpp file, not the h file (since the cpp is where XImpl is known, not just declared). I'm not sure if this could be incorporated as a policy into a general Pimpl, or should be kept somewhat separate. Maybe just same namespace or something. Tony
Gottlob Frege <gottlobfrege <at> gmail.com> writes: ... I have/had a SBO "DeplorablePimpl" template (see Herb Sutter's http://www.gotw.ca/gotw/028.htm). Basically
class X { DeplorableImpl<42, XImpl> pimpl; };
It takes care of most/all of Sutter's concerns. For example, it checks at compile time (in the cpp file) that sizeof(XImpl) == 42, (or <= 42, depending on policies) and forces correct constructors, etc. It does mean that X needs a constructor and destructor to be defined in the cpp file, not the h file (since the cpp is where XImpl is known, not just declared).
I'm not sure if this could be incorporated as a policy into a general Pimpl, or should be kept somewhat separate. Maybe just same namespace or something.
Oh, yes, I remember this one. :-) I think it might came up during original few-years-old discussion. Back then I remember I was somewhat apprehensive of this design. Now that I am growing old, cuddly and agreeable I do feel that this design has its place... and I dare to say if not prominent then at least an equal place among others. I do not see anything wrong with an optimization if the project specifics warrant/demand that. Say, OTOH for short-lived classes with small XImpl that'd be a *considerable* performance boost. If "my dear" Pimpl survives, I'll be looking into including it as a policy. many thanks, V.
On 27.05.2011 0:31, Sergiu Dotenco wrote:
Obviously, templates are not instantiated in the destructor, are they?
In the class template instantiation process only class is instantiated (without mem-funs). Mem-funs are instantiated in the call point, if any. In a non-inline ~Foo() (where ~data() is called) /struct data/ is complete type. Explicit (full) instantiation is provided by syntax like: template class std::auto_ptr<data>;
Am 27.05.2011 08:43, schrieb Max Sobolev:
On 27.05.2011 0:31, Sergiu Dotenco wrote:
Obviously, templates are not instantiated in the destructor, are they?
In the class template instantiation process only class is instantiated (without mem-funs). Mem-funs are instantiated in the call point, if any. In a non-inline ~Foo() (where ~data() is called) /struct data/ is complete type.
Explicit (full) instantiation is provided by syntax like:
template class std::auto_ptr<data>;
I'm not sure what your point is. Or is this just a comment?
participants (10)
-
Artyom Beilis -
Gottlob Frege -
Gregory Crosswhite -
Krzysztof Czainski -
Max Sobolev -
Sergiu Dotenco -
Stephan T. Lavavej -
Stewart, Robert -
Thomas Klimpel -
Vladimir Batov