[review][pimpl_ptr] Pimpl Pointer review begins
The review of the pimpl_ptr library by Asger Mangaard starts today, Monday May 15th, 2006, and runs through May 25th, 2006. (Boost Reviews RSS feed <http://tinyurl.com/ogpj6>) Pimpl Pointer ------------- :Author: Asger Mangaard :Download: Boost Sandbox (http://boost-consulting.com/vault/) as "pimpl_ptr.zip". Direct download at <http://tinyurl.com/m66ox>. :Description: The pimpl idiom is widely used to reduce compile times and disable code coupling. It does so by moving private parts of a class from the .hpp file to the .cpp file. However, it's implementation can be tricky, and with many pitfalls (especially regarding memory management). The pimpl_ptr library is a single header file, implementing a special policy based smart pointer to greatly ease the implementation of the pimpl idiom. Review questions ---------------- Please always explicitly state in your review, whether you think the library should be accepted into Boost. You might want to comment on the following questions: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? Enjoy, Rene. -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim - grafikrobot/yahoo
- What is your evaluation of the implementation?
The code doesn't compile with gcc, because getNew() should be this->getNew(). The 'inline' keywords are useless (implicit inline), at least on conforming compilers. The 'get()' method isn't const correct, e.g. you can write: void CGamePlayer::IncreaseLives() const { ++m_Values.get().m_Lives; }
- What is your evaluation of the documentation?
Both examples don't compile because of typos. (Ctest -> CTest, CgamePlayerValues -> CGamePlayerValues and more.) Why there's a C prefix?
- What is your evaluation of the potential usefulness of the library?
Unsure.
- Did you try to use the library? With what compiler?
Yes. GCC4, GCC3.
Did you have any problems?
Yes, see above and below.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A glance.
- Are you knowledgeable about the problem domain?
Yes. ---- I'm not sure what this utility is trying to address. The motivation is not to write down constructor, destructor, assignment operator and so on. But, unfortunately, you have to. Smart pointer or not, with a pimpl, the implicit generated versions of these operations are all wrong, because the compiler only has a forward declaration to the hidden class and therefore can't create, destroy and compare it. Even with some smart pointer you have to write down those operations, while leaving the function bodies in the cpp file empty. Sorry, my vote is: No. David.
David Gruener wrote:
- What is your evaluation of the implementation?
The code doesn't compile with gcc, because getNew() should be this->getNew().
Could you please specify which version of compilers it doesn't work on, to help the author out in testing and fixing it ;-) -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim - grafikrobot/yahoo
I've uploaded an updated version to address some of your issues.
The code doesn't compile with gcc, because getNew() should be this->getNew(). I'm very sorry about that. It's fixed in the latest version.
The 'inline' keywords are useless (implicit inline), at least on conforming compilers. The inline keyword is used to allow compliance also with non-conforming compilers.
Both examples don't compile because of typos. (Ctest -> CTest, CgamePlayerValues -> CGamePlayerValues and more.) Why there's a C prefix?
Fixed. Sorry. The C prefix is just the code convention I've always used. If it's vital, I'll change it.
Asger Mangaard wrote:
The 'inline' keywords are useless (implicit inline), at least on conforming compilers.
The inline keyword is used to allow compliance also with non-conforming compilers.
I'm curious: which compiler? I don't see any boost library following this convention.
Fixed. Sorry. The C prefix is just the code convention I've always used. If it's vital, I'll change it.
Ok. :-) But the most important issue is the motivation, i.e. you still need to provide (empty) definitions members and functions. Thus, for me, this utility doesn't by much beside the const correctness. (You need an indirection when using raw pointer or reference.) I prefer stating e.g., Foo::Foo(Foo const& other) : priv_(*other.priv_) { } instead of _empty and required_ Ctors and functions, but others might think different. If you can address the issue of the out-of-line definitions in any way, that would be the step in the right direction, while i feel that this is not possible with a greedy approach, because it always boils down to the point that the implementation file holds the struct. David.
David Gruener wrote:
I'm not sure what this utility is trying to address. The motivation is not to write down constructor, destructor, assignment operator and so on. But, unfortunately, you have to. Why do you think that? No, you don't have to write a constructor, destructor, or an assigment operator for the pimpl_ptr to work. Please look at the example/default.cpp file.
Smart pointer or not, with a pimpl, the implicit generated versions of these operations are all wrong, because the compiler only has a forward declaration to the hidden class and therefore can't create, destroy and compare it. Again, I don't know why you think this it true - unless I'm missing something. When constructed/destructed the T type isn't undefined since it's defined in the cpp file. That's the fact I've build this library on top of.
Even with some smart pointer you have to write down those operations, while leaving the function bodies in the cpp file empty. Still I don't agree with you. Do you have an example to showcase this?
Regards, Asger
Asger Mangaard wrote:
David Gruener wrote:
I'm not sure what this utility is trying to address. The motivation is not to write down constructor, destructor, assignment operator and so on. But, unfortunately, you have to.
Why do you think that? No, you don't have to write a constructor,
You have to, and you should read the pointers you are given. (here: The article with the grin pointer.)
Still I don't agree with you. Do you have an example to showcase this?
Simple. Just try to use (create, detroy, copy...) a pimpled class with implicit defined functions somewhere else than in the implementation file. hth, David.
David Gruener wrote:
I'm not sure what this utility is trying to address. The motivation is not to write down constructor, destructor, assignment operator and so on. But, unfortunately, you have to.
Why do you think that? No, you don't have to write a constructor,
You have to, and you should read the pointers you are given. (here: The article with the grin pointer.)
Still I don't agree with you. Do you have an example to showcase this?
Simple. Just try to use (create, detroy, copy...) a pimpled class with implicit defined functions somewhere else than in the implementation file.
Could you please provide an example? I don't see your point when I'm providing code and examples that does what you claim it can't. Maybe I'm missing something? Could I add; I'm not in any way trying to be rude, I'm just trying to understand your points. Regards, Asger
"Asger Mangaard" <tmb@tmbproductions.com> writes:
David Gruener wrote:
I'm not sure what this utility is trying to address. The motivation is not to write down constructor, destructor, assignment operator and so on. But, unfortunately, you have to.
Why do you think that? No, you don't have to write a constructor,
You have to, and you should read the pointers you are given. (here: The article with the grin pointer.)
Still I don't agree with you. Do you have an example to showcase this?
Simple. Just try to use (create, detroy, copy...) a pimpled class with implicit defined functions somewhere else than in the implementation file.
Could you please provide an example? I don't see your point when I'm providing code and examples that does what you claim it can't.
Maybe I'm missing something?
Could I add; I'm not in any way trying to be rude, I'm just trying to understand your points.
//test.hpp #include "boost/pimpl_ptr.hpp" class test { public: void do_stuff(); private: boost::pimpl_ptr<struct test_implementation> pimpl; }; //test_impl.cpp #include "test.hpp" struct test_implementation { void really_do_stuff(){} }; void test::do_stuff() { pimpl->really_do_stuff(); } //test_usage.cpp #include "test.hpp" int main() { test x; // Error, test_implementation incomplete. x.do_stuff(); } Anthony -- Anthony Williams Software Developer Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk
"Asger Mangaard" wrote:
Why do you think that? No, you don't have to write a constructor, destructor, or an assigment operator for the pimpl_ptr to work. Please look at the example/default.cpp file.
Create additional *.cpp file, include the "pimpl_ptr.hpp" and try to use it. Compiler error follows. /Pavel
On Thu, May 18, 2006 at 01:06:46PM +0200, Pavel Vozenilek wrote:
"Asger Mangaard" wrote:
Why do you think that? No, you don't have to write a constructor, destructor, or an assigment operator for the pimpl_ptr to work. Please look at the example/default.cpp file.
Create additional *.cpp file, include the "pimpl_ptr.hpp" and try to use it. Compiler error follows.
That shouldn't be the case if pimpl_ptr never tries to construct the templated type in it's header. Which compiler/version is everyone using? Maybe one or the other is broken. This should compile fine: ---------- template<typename T> struct pimpl_ptr { T* ptr; // Only pointers allowed. }; struct test_impl; struct test { pimpl_ptr<test_impl> ptr; }; int main() { test t; // OK } ------------ While this should indeed give an error: ------------ template<typename T> struct pimpl_ptr { T ptr; // Actually constructing type T. }; struct test_impl; struct test { pimpl_ptr<test_impl> ptr; }; int main() { test t; // Error, 'test_impl' incomplete } ------------ -- Carlo Wood <carlo@alinoe.com>
Rene Rivera wrote:
- Are you knowledgeable about the problem domain? - What is your evaluation of the potential usefulness of the library? - What is your evaluation of the design?
As someone who uses the handle/body idiom quite frequently, I'd say it can be very useful. But o for noncopyable classes (90% of the "pimpl candidates" in my projects) scoped_ptr works fine o in other places shared_ptr or intrusive_ptr work, but needs extra work to implement deep copy plus with shared_ptr there is unecessary overhead for reference counting and with intrusive_ptr it needs two no-op functions to bypass reference counting. Brings us to the question: What can pimpl_ptr give us that Boost.SmarPtr can't? The answer to this question with the current state of the library is: o save a few lines of code for deep copy o save a few lines for "deep compare" (it seems unlikely to me that this feature will be very useful) Next question is: What could an imaginary pimpl_ptr give us that Boost.SmartPtr can't? o stack allocation using SBO o COW (copy on non const access) o both of the above (these are the features I had actually hoped for) I have a bad feeling about the policies, especailly the first two of them. "lazy creation" could be interesting, but I'm not too sure about that.
- What is your evaluation of the documentation?
I'd prefer body_ptr for the name, because: o it makes some sense to those who never heard about the pimpl idiom o it sounds much sexier ;-) The layout does not really hit my taste, that is I find it hard to read. Especially the reference needs to be spaced out. Further, I don't like the upper case names used in the examples (I guess it's just a matter of taste, still, it's Boost style). The code in the tutorial can probalby use some abbreviation. And I think you should compare the library with other Boost smart pointers rather than with hand-written code. boost::pimpl_ptr<struct CPrivateMembers*> m_pImpl; in the client class, where pimpl_ptr obviously adds another pointer to T pimpl_ptr( T* _pDefault); seems like a typo (one that can cause serious confusion).
- What is your evaluation of the implementation?
Didn't look.
- Did you try to use the library? With what compiler? Did you have any problems?
No.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A quick reading.
Please always explicitly state in your review, whether you think the library should be accepted into Boost.
I vote not to accept this library, at lest not in its current state. Regards, Tobias
Tobias Schwinger wrote:
As someone who uses the handle/body idiom quite frequently, I'd say it can be very useful.
But o for noncopyable classes (90% of the "pimpl candidates" in my projects) scoped_ptr works fine o in other places shared_ptr or intrusive_ptr work, but needs extra work to implement deep copy
No. The boost smart pointers are _not_ directly suitable for pimpls, because they don't propagate const. David.
David Gruener wrote:
Tobias Schwinger wrote:
As someone who uses the handle/body idiom quite frequently, I'd say it can be very useful.
But o for noncopyable classes (90% of the "pimpl candidates" in my projects) scoped_ptr works fine o in other places shared_ptr or intrusive_ptr work, but needs extra work to implement deep copy
No. The boost smart pointers are _not_ directly suitable for pimpls, because they don't propagate const.
This also makes them less useful in other contexts IMO. Just because I use vector<shared_ptr<T>>, doesn't mean that I want to give up const-correctness. And there is no efficent way to convert vector<shared_ptr<T>> to vector<share_ptr<const T>>. -Thorsten
Tobias Schwinger wrote:
o save a few lines for "deep compare" (it seems unlikely to me that this feature will be very useful) Would you rather compare on the pointers themselves, or?
o stack allocation using SBO Could be interresting.
o COW (copy on non const access) Could you give an example?
seems like a typo (one that can cause serious confusion). Thank you for finding these.
Regards, Asger
Asger Mangaard wrote:
Tobias Schwinger wrote:
o save a few lines for "deep compare" (it seems unlikely to me that this feature will be very useful)
Unfortunately the context got lost, here: That point is from my summary of what the library seems to buy me in its current state.
Would you rather compare on the pointers themselves, or?
Not neccessarily (but it seems a more approriate default to me, while we're at it). Why bother with comparison in the first place?
o stack allocation using SBO
Could be interresting.
...and can probably done with STL-style allocator support. Something like that namespace my_project { // customize boost::body_ptr for my_project's memory management template<typename T> struct body_ptr : boost::body_ptr<T, my_custom_allocator<T> > { }; } should work, however, so the user does not have to repeat the parametrization all over the place.
o COW (copy on non const access)
Could you give an example?
Let's see (untested and incomplete code for illustration only): class my_class { struct heavy_body; body_ptr< heavy_body > ptr_body; public: state get_state() const; void set_state(state); }; // ... my_class my_instance; // ... my_class my_second_instance(my_instance); // no copy, both instances share the same body #if case1 my_second_instance.set_state(some_state); // non-const access of shared body ==> need a copy // my_second_instance gets a brand new copy #elif case2 my_instance.set_state(some_state); // non-const access of shared body ==> need a copy // my_instance gets a brand new copy #endif Works for you? Note that you have to synchronize the reference counter and also the copy operation in a multithreaded environment. Regards, Tobias
Tobias Schwinger wrote:
Could you give an example?
Let's see (untested and incomplete code for illustration only): ....
It seems like you assume that two pimpl_ptr instances can hold the same pointer. This is _not_ the case. Each instance of pimpl_ptr will have its own instance of T, thus the COW (copy on non const access) isn't needed. Regards, Asger
Asger Mangaard wrote:
Tobias Schwinger wrote:
Could you give an example?
Let's see (untested and incomplete code for illustration only): ....
It seems like you assume that two pimpl_ptr instances can hold the same pointer. This is _not_ the case.
Why not?
Each instance of pimpl_ptr will have its own instance of T, thus the COW (copy on non const access) isn't needed.
COW (copy one write, which is what it actually stands for) is a common optimization technique. It's never /needed/ it just makes things faster ;-). Regards, Tobias
Tobias Schwinger wrote:
Asger Mangaard wrote:
Tobias Schwinger wrote:
Could you give an example?
Let's see (untested and incomplete code for illustration only): ....
It seems like you assume that two pimpl_ptr instances can hold the same pointer. This is _not_ the case.
Why not?
From my experience, you don't because at that point you would use some other kind of pointer. Most likely a reference counted pointer that can travel across DLL boundaries. I think we all have to keep in mind the scope of this utility. It not meant to solve all the pointer uses. There's another utility that people keep asking for, the traits pointer. -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim - grafikrobot/yahoo
Rene Rivera wrote:
I think we all have to keep in mind the scope of this utility. It not meant to solve all the pointer uses.
I understand your concern, still, in my eyes COW is just another application of the handle/body pattern and thus not /that/ far away from the scope of this submission.
There's another utility that people keep asking for, the traits pointer.
Whether a single policy-based can-do-whatever-pointer is really what we want is a different story.
It seems like you assume that two pimpl_ptr instances can hold the same pointer. This is _not_ the case.
Why not?
From my experience, you don't because at that point you would use some other kind of pointer. Most likely a reference counted pointer that can travel across DLL boundaries.
<cite> However, it's implementation can be tricky, and with many pitfalls (especially regarding memory management). </cite> template<typename T> void dispose_func(void* ptr) { delete static_cast<T*>(ptr); } // use void(*)(void*) to this thing for freeing the shared context // and the instance Regards, Tobias
Tobias Schwinger wrote:
Rene Rivera wrote:
I think we all have to keep in mind the scope of this utility. It not meant to solve all the pointer uses.
I understand your concern, still, in my eyes COW is just another application of the handle/body pattern and thus not /that/ far away from the scope of this submission. [...]
From my experience, you don't because at that point you would use some other kind of pointer. Most likely a reference counted pointer that can travel across DLL boundaries. template<typename T> void dispose_func(void* ptr) { delete static_cast<T*>(ptr); }
Yes I do know how the DLL boundary issue is solve ;-) My point is the reference counting... What is being suggested, by you and others, is to have a reference counted pointer with deep copy semantics implemented as copy on write for efficiency. And in my opinion at that point you are looking at the territory of shared_ptr/weak_ptr, and hence outside of the intended scope of the pimpl_ptr submission. -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim - grafikrobot/yahoo
Rene Rivera wrote:
Tobias Schwinger wrote:
Rene Rivera wrote:
I think we all have to keep in mind the scope of this utility. It not meant to solve all the pointer uses.
I understand your concern, still, in my eyes COW is just another application of the handle/body pattern and thus not /that/ far away from the scope of this submission.
[...]
From my experience, you don't because at that point you would use some other kind of pointer. Most likely a reference counted pointer that can travel across DLL boundaries.
template<typename T> void dispose_func(void* ptr) { delete static_cast<T*>(ptr); }
Yes I do know how the DLL boundary issue is solve ;-)
I figured /you/ do, but the issue deserved public demystification ;-).
My point is the reference counting...
Yes, I do understand your point, that is looking from the perspective of someone who implements a smart pointer.
What is being suggested, by you and others, is to have a reference counted pointer with deep copy semantics implemented as copy on write for efficiency.
From a user's perspective, however, it makes sense that an opaque smart pointer intended for Handle/Body is implemented as copy on write in some cases (and it is completely unimportant to me whether the copy on write implementation needs reference counting to get the job done):
Let's say I use the pimpl idiom in an imaginary application. I use a Boost utility and (as usual) I'm quite happy with it. Some time later (say, the application has been released) it turns out that things should run faster and my profiler tells me that (among other things) the application spends too much time copying pimplEd classes. At this point I'm very thankful, if all I have to do is switch the smart pointer to a different memory mangement policy, rather than being left on my own. Note that especially classes that are expensive to copy tend to be good candidates for both pimpl and COW, so optimization will not affect a well-done design. And (as stated in my previous post) both pimpl and COW are applications of the very same design pattern, namely Handle/Body (or Bridge using Go4 terminology).
And in my opinion at that point you are looking at the territory of shared_ptr/weak_ptr,
If so (see above)...
and hence outside of the intended scope of the pimpl_ptr submission.
...the scope might be too narrow. Regards, Tobias
"Tobias Schwinger" wrote: [ COW implementation of pimpl ]
Note that you have to synchronize the reference counter and also the copy operation in a multithreaded environment.
If a read-write lock will be added it should be optional - the overhead may be too big. It should be also possible to _manually_ trigger the cloning: * it the pimpl is passed into another hread and we do not want to deal with locking * if the member is const but changes a mutable data and the design cannot be changed. /Pavel
On 5/19/06, Pavel Vozenilek <pavel_vozenilek@hotmail.com> wrote:
If a read-write lock will be added it should be optional - the overhead may be too big.
It should be also possible to _manually_ trigger the cloning:
The Adobe Open Source libraries have a copy_on_write<T> that implements this nicely. There is a method called write() which triggers the copy operation and returns a ref to the newly cloned object. The copy is only performed if there is more than one reference to the object. adobe::copy_on_write<int> first (42); adobe::copy_on_write<int> second (first); assert (first.identity (second)); // same object first.write() = 56; // a copy is made, and a ref to it is returned assert (!first.identity(second)); assert (first.unique_instance()); assert (second.unique_instance()); first.write() = 42; // no copy, since first is a unique instance http://opensource.adobe.com/classadobe_1_1copy__on__write.html -- Caleb Epstein caleb dot epstein at gmail dot com
Summary: I vote NO to including pimpl_ptr in boost. There is a better implementation (entitled grin_ptr) available from Alan Griffiths' website (http://www.octopull.demon.co.uk/c++-writing.html). Alan describes a new revision of his code in the April 2006 issue of Overload (http://accu.org/index.php/aboutus/latestjournals), but I don't think this is on his website as yet. Rene Rivera <grafik.list@redshift-software.com> writes:
- Are you knowledgeable about the problem domain?
Yes. I've written smart pointers before, and used the pimpl idiom. I am aware of the issues that surround such usage.
- What is your evaluation of the design?
The policy based design is overly complex. The lazy initialization feature is really orthogonal to the use of the pimpl idiom.
- What is your evaluation of the implementation?
The current implementation requires that the implementation type T be complete for the copy constructor, assignment operator and destructor, in order to perform the deep copies. Therefore classes that use pimpl will need to provide out-of-line definitions of these members, even if they are empty (which is the expected case).
- What is your evaluation of the documentation?
The documentation seems to clearly explain the intended usage, though there are a few typos (e.g. inconsistent case in the class names/constructor names in the example).
- What is your evaluation of the potential usefulness of the library?
Reasonable. Pimpl is a relatively common idiom, and it would be good to have a boost library that addressed the appropriate issues.
- Did you try to use the library? With what compiler?
Yes. g++ 4.0.1
Did you have any problems?
Yes. pimpl_ptr.hpp line 129 needs getNew() to be qualified with this->, since it is a non-dependent name, but it is defined in a dependent base class. As mentioned above, the implementation doesn't match the docs, and the gameplayer example from the docs doesn't compile, as the compiler-generated members of CGamePlayer need CGamePlayerValues to be a complete type.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Reasonably in-depth study of code. Quick glance through docs. Anthony -- Anthony Williams Software Developer Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk
Before I go into detail - which I'm sorry to say is a bit critical - I'd like to say that I'm in favour of a pimpl ptr. Boost's Smart Ptr's don't quite give what I need. In particular the Smart Ptr library's shallow-constness changes the semantics of a class when you introduce a pimpl. This is a common idiom, and it should be expressed in boost.
- What is your evaluation of the design?
It seems to be overly complex in some areas. For example I don't think the comparison operators are going to be useful enough to justify the extra complexity they cause. If its felt that they can be useful, I certainly don't think they need to be templated on the the RHS type. A pimpl ptr is used to hide the implementation of a class, so why would it need to be compared with an implementation of another type? Does it actually need policies? How much use are they? I'm not strongly opinioned either way, but it needs to be discussed. Assuming its decided that policies are needed, do they need to be a class template? Can they not be implemented using member function templates? The docs claim that pimpl_ptr is extendable through policies, but since the policy base class (which I would derive from?) is in detail this would seem to be prevented (well, discouraged anyway). For example I can think of at least two other policies : 1) Custom deleter. This would also allow the removal of the destructor from the class that would use it. And the passing of objects with pimpls over DLL boundaries. 2) Static storage. In certain, very rare, cases I can't afford the overhead of a heap allocation. But I still want the benefits of a pimpl. So a static_storage<size_t Size> policy would be useful. The policy would of course check at compile time that Size was correct. [1] The first could probably be written, but the second can't be done in the class template form, because the template template parameters wouldn't match correctly. Does it need to be copyable? or assignable? I'm fairly sure it doesn't need to be copy constructible and assignable from a templated impl_ptr.
- What is your evaluation of the implementation?
In general it is good, a few minor issues. Names beginning with _ should be avoided. Although the standard says they're ok (17.4.3.1.2), not all library vendors have got it right, so they're worth avoiding. Like I said earlier, I don't think the policies should be class templates. I don't think the copy constructors should be calling T::operator=. They should call base::base(new T(*rhs_.copy())) detail::base::get() provides a non-const reference, so it breaks the const-deepness that operator-> provides.
- What is your evaluation of the documentation?
The docs appear to have been written in Word. Using HTML tidy the docs dropped from 140kb to 34kb. By document section : Introduction: I'm not sure how much detail you need to go into the whys and hows of the pimpl idiom, can this be mentioned very briefly with a reference to other literature (such as [1]). The counter example - showing why pimpl_ptr helps - does more than the original example - op== and op!= are added. I think this section would be better served by showing how using pimpl_ptr is an improvement over shared/scoped_ptr. I think the majority of users that find pimpl_ptr will have already looked at SmartPtr or at least std::auto_ptr. Rationale: This section appears to give rationale for the library as a whole (ie concluding the introduction). Typically the rationale provides a rationale for individual (potentially) contentious design decisions. Such as : the policy design, including op==, op<. How it works: I think this section could be removed, it seems quite obvious to me and it adds nothing that the reference doesn't tell you. Policy Overview : The exact semantics for each policy need to be spelled out here. The current wording is a bit ambiguous. For example, the policy manual_creation says "will rely on the user to create/set the pointer" but nowhere is it specified how you would do that. Detailed Semantics: pimpl_ptr(T* ) in the notes : "Accepts a zero pointer. In this case the previous pImpl pointer will be deleted." Surely there cannot be a previous pointer? this is a constructor! operator < : makes a reference to operator!=, I think this is a copy-paste typo. Type requirements : missed out Assignable, and EqualityComparable, and LessThanComparable.
- What is your evaluation of the potential usefulness of the library?
Whilst not a critical library, I think it is a worthwhile one, as its a common idiom, that should be represented in boost.
- Did you try to use the library? With what compiler?
No, but I will do when the design is right.
Did you have any problems? n/a
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
An in depth reading of the code, examples, tests, and docs. But no attempt to use it.
- Are you knowledgeable about the problem domain?
Yes. I have written a very similar class many moons ago, and I've been using the pimpl idiom for years. ------------------ My vote is a weak No at the moment. But if : + the documentation was improved + base::get() was fixed + the exact specification for policies was specified. Then I would vote a weak yes. And if the following changes were made : + policies were changed so they were not class templates. + the comparison operators were removed + the copy constructor, the assignment operator, and swap were un-templated. Then I would vote a strong yes. Sam [1] Yes I know many, including herb are dead against this technique : http://www.gotw.ca/gotw/028.htm. But I still maintain from time to time it is something I would need/want to do, so it should be possible for me to write a policy to do that. [2] http://www.gotw.ca/gotw/024.htm
"Rene Rivera" wrote:
The review of the pimpl_ptr library by Asger Mangaard starts today,
I vote no on the library in current state. (the problem addressed by David Gruener, docs could be much, much better, etc) I believe there's huge potential if scope of the library is expanded to provide "resource management" functionality. The lazy_creation is interesting start and I can imagine techniques as: * body is self-deleted after timeout when not used * pool of objects * serialization of objects creation in MT environment (I have constructor that cannot be MT safe and would like the library to protect against multiple constructors/destructors running at the same time. * other features suggested bellow. /Pavel _______________________________________________ 1. The font size of the HTML documentation is hardcoded and rather small. What should people with large resolutions do? _______________________________________________ 2. The part of introduction explaining pImpl should be omitted. Anyone curageous to use Boost knows it. Provide a link. _______________________________________________ 3. Color syntax highlighting should be used. _______________________________________________ 4. The funny "C" prefix should be removed and the standard "name_with_underscore" convention used. _______________________________________________ 5. The policies should not be in detail namespace and definitely should be documented. _______________________________________________ 6. Description for "manual_creation" policy is insufficient and unclear. _______________________________________________ 7. A policy "lazy_creation_with_timeout" would be quite useful for real life projects. Likewise, ability to "un-set" the pointer manually may be useful. With these capabilities the library would go beyond simple pImpl but I think they would be quite natural here. _______________________________________________ 8. Examples should be moved on the top of the documentation and it should be possible to copy+cut and compile them. _______________________________________________ 9. Style of the code: the double or triple indentation namespace boost { namespace pimpls { namespace detail { here_we_start should be replaced with more readable namespace boost { namespace pimpls { namespace detail { here_we_start _______________________________________________ 10. Inlined function bodies are shorter and more readable than out-of-class definitions. _______________________________________________ 11. Naming: "_pDefault": leading underscore is reserved for compiler/standard lib. _______________________________________________ 12. In: T* p = new T; if ( !p ) { boost::checked_delete( p ); <<=== ???? .... Why is checked_delete on 0 pointer used? The type is complete (since constructor exists). _______________________________________________ 13. Style: instead of typing pimpl_ptr<T, this_policy> a specialization like: lazy_pimpl<T> would be more readable and less demanding on scrouring through the documentation. _______________________________________________ 14. Test source file contains tabs. _______________________________________________ 15. The pimpl_ptr.hpp is not in boost subdir, why should fiddle with directories manually? _______________________________________________ 16. When compiling under BCB 6.4 I got warning and error: [C++ Warning] pimpl_ptr_test.cpp(198): W8004 'oneParameter' is assigned a value that is never used Full parser context pimpl_ptr_test.cpp(104): parsing: int main(int,char * *) [C++ Error] stl/_algobase.h(75): E2015 Ambiguity between 'boost::void swap<CMyValues1,boost::pimpls::lazy_creation<T>,CMyValues1,boost::pimpls::lazy_creation<T>
(boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T> > &,boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T>
&)' and '_STL::void swap<boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T> >
(boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T> > &,boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T>
&)'
_______________________________________________ 17. Compiling and running the test under Intel C++ 9.0 plugged in VC6 IDE is OK. Also all examples work. _______________________________________________ 18. The manual_creation doesn't show why and how this feature could be used. _______________________________________________ 19. I think it is feasible to support parameter passing in pimpl constructor. _______________________________________________ 20. The issue addressed by David Gruener (incomplete types when using the pimpled class in other translation unit) needs to be addressed. _______________________________________________ 21. Support for the unsafe and much discouraged hackery with fixed size buffer where the body is constructed would be useful at a times. Support of static storage for the body may be useful for those giant classes usually named "toolkit". The library may contain code checking that just single instance of the body is ever created. _______________________________________________ 22. The feaures suggested by Tobias Schwinger would be very handy. _______________________________________________ 23. Possible feature: provide support for several bodies that are composed into single unified interface: class handle { ... body1* pimpl1; body2* pimpl2; ... }; to: class handle { .... pimpl_ptr<struct Body1, struct Body2> pimpl }; void handle::foo() { pimpl[0]->foo(); } _______________________________________________ 24. Feature: provide ability to "switch" class type dynamically (as if virtual table is replaced). The "set()" could be such feature but it is not documented enough and the name is not descriptive. The switch should be possible from within the body (the most common scenario). _______________________________________________ 25. Feature: provide wrapper that intercepts certain exceptions and switches to callback. Example: two body implementations are provided. If function from body1 throws this or that exception it is intercepted, ignored and instead result of call into body2 is returned. _______________________________________________ 26. Feature: provide well documented framework so people could add their own specialised pimpls (e.g. every method runs in its own thread, if timeout expires a default value is returned). The features suggested above could be used as examples of such framework. _______________________________________________ EOF
On Monday 15 May 2006 02:45, Rene Rivera wrote:
Direct download at <http://tinyurl.com/m66ox>. [..] - What is your evaluation of the design?
Read through it, no in-depth analysis. I mostly like it, in particular that there is a ready-made implementation for lazy initialisation.
- What is your evaluation of the implementation?
Read through it, no in-depth analysis. There are a few things that struck me: - There are several memberfunctions or class detail::base that are declared as inline and outside the class body, while others miss this. Why not declare them inside the class, so they get the inline implicitly? Less typing, less reading, easier maintenance, same binary code. - I saw this code: T* p = new T; if ( !p ) {...} but new will throw when allocation fails. IFF something like that was done for broken compilers/standardlibraries, then it should be documented. I'd even say it should then go into a boost-wide systematic workaround, encapsulated in a macro that evaluates to nothing in conformant environments. - Examples use FOO__H as include guard, which is invalid because it contains two consecutive underscores. - I wouldn't provide operators !=, == and <. These need manual intervention to forward them to the pimpl anyway and then it is just a little step from writing "return lhs.impl==rhs.impl;" to "return *lhs.impl==*rhs.impl;". - I wouldn't provide the templated constructors and operator. Someone already mentioned that this makes no sense when the implementation is hidden, which includes hidden from other implementations. Anyhow, what other implementations would you want to compare with? If it's the same type, it doesn't matter, if a class has different implementations, I wouldn't exactly call this a PIMPL. - operator* is missing. - Could use a safe_bool conversion, in particular for delayed initialisation.
- What is your evaluation of the documentation?
Notes: - The transitivity of const is IMHO another goal of such a library, i.e. that a const pimpl only allows const access to the implementation. - A repetition of the assumption that new could return null. - Calling an object 'uninitialized' after its constructor ran is confusing ("Postconditions: *this is uninitialized."). - Documentation should mention that all functions that need to know the full type of the implementation (i.e. ctor, dtor..) must not be inline. Examples should reflect this, there is no use for a PIMPL when everything is in a single file anyway. - The use of an on-demand created object in vector it related but still IMHO completely outside the scope of a PIMPL library. I would remove this use from the design goals and documentation and even make the pimpl_ptr noncopyable. If the final design still say it can be used for this, I would also mention the alternative of using Boost.Optional, which acts similar.
- What is your evaluation of the potential usefulness of the library?
Medium, though I must underline the 'potential' in this statement. The current state needs some more work. Other than that (and the reason for the 'medium') PIMPL is just too simple to get right, in particular since often these complex classes are entity classes and as such noncopyable, reducing the effort to ctor and dtor.
- Did you try to use the library?
No.
With what compiler?
N/A.
Did you have any problems?
N/A.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Around 90 minutes of reading the library and writing this mail.
- Are you knowledgeable about the problem domain?
I used the PIMPL pattern before and followed discussions on the topic. Uli
On Thursday 25 May 2006 09:10, Ulrich Eckhardt wrote:
- What is your evaluation of the design?
Read through it, no in-depth analysis. I mostly like it, in particular that there is a ready-made implementation for lazy initialisation.
Rene pointed out in a private mail that I didn't vote for inclusion nor against. I vote against inclusion (but I'd be all too happy to see an overhauled version). Uli
participants (12)
-
Anthony Williams -
Asger Mangaard -
Caleb Epstein -
Carlo Wood -
David Gruener -
Pavel Vozenilek -
Rene Rivera -
Sam Partington -
Thorsten Ottosen -
Tobias Schwinger -
Ulrich Eckhardt -
Ulrich Eckhardt