I would like to submit the pimpl library for a formal review. The suggested library can be found in the sandbox's root. Regards, Asger Mangaard
Hi Asger Asger Mangaard wrote:
I would like to submit the pimpl library for a formal review. The suggested library can be found in the sandbox's root.
After looking at the submission briefly (2 minutes), I'm a bit at a loss exactly how I'm supposed to use it. You might want to consider adding at least a motivating example (reference and rationale documentation is usually also part of a submission for review). IMHO, people interested in the library should not be forced to search the mailing list archive or infer the usage from the code. Regards, -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.
On Sat, 8 Oct 2005 15:21:11 +0200, Andreas Huber wrote
Hi Asger
Asger Mangaard wrote:
I would like to submit the pimpl library for a formal review. The suggested library can be found in the sandbox's root.
After looking at the submission briefly (2 minutes), I'm a bit at a loss exactly how I'm supposed to use it. You might want to consider adding at least a motivating example (reference and rationale documentation is usually also part of a submission for review). IMHO, people interested in the library should not be forced to search the mailing list archive or infer the usage from the code.
The files in the sandbox are insufficient for a formal review request. They don't contain any real documenation (pimpl.txt is really just some notes). Documentation is essential and reviewers need to be able to read the library motivation and use the submission without reading the code. Jeff
I've uploaded a new suggestion, pimpl_ptr.zip with the name changed from pimpl to pimpl_ptr. Also included is a lot more documentation. Regards and tell me what you think, Asger Mangaard
Asger Mangaard wrote:
I've uploaded a new suggestion, pimpl_ptr.zip with the name changed from pimpl to pimpl_ptr. Also included is a lot more documentation.
IMHO, even for such a small library that's still far from enough for formal review. For an example of what I would consider sufficient, have a look at the documentation of the optional library: http://www.boost.org/libs/optional/doc/optional.html Also, you might want to consider... - that HTML is somewhat standard for boost documentation. - that prepending identifiers with underscores makes your code non-portable. - to add swap(). - to provide the strong exception guarantee for all functions. - to put everything in established standard directories (e.g. doc, test). - to add bjam files for you tests. Feel free to have a look at code, docs & tests of other boost libraries to get a feeling what is commonly considered sufficient. Regards, -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.
"Andreas Huber" <ahd6974-spamgroupstrap@yahoo.com> writes:
- that HTML is somewhat standard for boost documentation.
Yep.
- that prepending identifiers with underscores makes your code non-portable.
Only if they're in the global namespace or followed by a capital letter, right?
- to add swap().
That's a nice thing to do.
- to provide the strong exception guarantee for all functions.
That's bad advice, in general. Provide the basic guarantee for all functions. Provide the strong guarantee where possible without loss of efficiency.
- to put everything in established standard directories (e.g. doc, test).
Yep.
- to add bjam files for you tests.
Yep.
Feel free to have a look at code, docs & tests of other boost libraries to get a feeling what is commonly considered sufficient.
Good idea. -- Dave Abrahams Boost Consulting www.boost-consulting.com
Hi Dave David Abrahams <dave <at> boost-consulting.com> writes:
Only if they're in the global namespace or followed by a capital letter, right?
Hmmm, ok, by that definition I don't remember seeing any non-portable uses of leading underscores. I guess it's just unusual style then :-).
- to provide the strong exception guarantee for all functions.
That's bad advice, in general. Provide the basic guarantee for all functions. Provide the strong guarantee where possible without loss of efficiency.
Right, that advice was meant specifically for the pimpl_ptr library. AFAICS, providing the strong guarantee will cost close to nothing. Regards, -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.
After 2 weeks of writing documentation, creating tests, etc. I've uploaded a new zip file to the file vault. What's new since last: * HTML documentation * In depth documentation * boost::swap support * uses the boost directory structure * examples w. jam files * test w. jam files * small fixes that came to mind while writing the documentation I hope for comments and suggestions. Sincerly, Asger Mangaard
I use the pimpl idiom quite a bit and have followed the discussion on this proposed library. Here's some comments from looking at the just uploaded version (I didn't look at any of the previous versions in any kind of depth): 1. The Motivation and Rationale sections of the documentation needs to be much more detailed, comprehensive, and clear before I will use this library. To me, this is crucial for this library, since there must be clear advantages over existing practices and techniques (I think this is true of most "idiom" based libraries, versus libraries fulfilling a specific functionality need). -- Summarize Sutter's treatment of pimpl's, and compare and contrast some of the existing pimpl approaches. Provide a reference / link to Sutter's articles and books(http://www.gotw.ca/publications/mill04.htm). -- Summarize some of the disadvantages: performance will decrease (methods cannot be inlined, additional heap allocation and deallocation is required, an extra indirection is required for each method invocation) and there will be increased heap usage (which can be an issue for embedded systems). -- One of the primary uses for pimpl's in my experience is in hiding 3rd party or OS headers from the application using my class. This advantage should not be underestimated / underdocumented, specially for large system development, or for writing libraries that will be used on multiple platforms / environments. -- Contrast your library with using boost::shared_ptr or boost::scoped_ptr - in particular, both shared and scoped ptrs support incomplete type usage (in the header). Right now, the only advantage I see in using pimpl_ptr versus shared / scoped ptr is in the policy behavior - lazy creation, etc. Will this advantage go away once/if a policy ptr is accepted into Boost? 2. Additional examples would be very helpful, and shouldn't be too hard to produce. The two existing examples should have some comments and / or additional code to point out the advantages of using pimpl_ptr. 3. A documentation re-write for improved grammar and writing style is needed. 4. "In general" doesn't seem precise for the type requirements. Either type T must be copy constructible or not, or specify the requirements by pimpl_ptr methods / use. The tests should enforce the requirements. Also, looking at the code, there's at least a couple of areas where assignment is required for the contained type T (e.g. pimpl_ptr::operator=) and "operator==" is required for the contained type in at least two places. 5. Looking at the code, the pimpl_ptr copy constructor is performing assignment in the ctor body ... how is the deep copy being performed? I don't see deep copy semantics in either the "base" class or the policy classes. Does this affect the requirements of the contained type T? 6. If a pimpl_ptr is default constructed with a manual_creation policy, how is the impl object created / passed in at a later time? 7. One of the issues with policy based template classes is compatibility / conversion between types with different policies. In this library, pimpl_ptr<Foo> and pimpl_ptr<Foo, lazy_creation> are different types, and it's not clear to me how / if they can interact with each other. 8. Swap is not swapping policies, only the underlying impl pointer (unless I'm confused). Hope this helps, Cliff
After almost two months I've finally uploaded an updated version.
Cliff wrote:
1. The Motivation and Rationale sections of the documentation needs to be much more detailed, comprehensive, and clear before I will use this library. To me, this is crucial for this library, since there must be clear advantages over existing practices and techniques (I think this is true of most "idiom" based libraries, versus libraries fulfilling a specific functionality need). I've tried hard to meet these requirements in the update.
-- Contrast your library with using boost::shared_ptr or boost::scoped_ptr - in particular, both shared and scoped ptrs support incomplete type usage (in the header). Right now, the only advantage I see in using pimpl_ptr versus shared / scoped ptr is in the policy behavior - lazy creation, etc. Will this advantage go away once/if a policy ptr is accepted into Boost? I've included an example called 'compare_to_other.cpp' which should show the advantages compared to other smart pointers. Should a policy_ptr be introduced to boost, I'll have to revise my work.
3. A documentation re-write for improved grammar and writing style is needed. I'm from Denmark, and English isn't my first language. Still I'm sorry if my english is really that bad :(
4. "In general" doesn't seem precise for the type requirements. Either type T must be copy constructible or not, or specify the requirements by pimpl_ptr methods / use. The tests should enforce the requirements. Also, looking at the code, there's at least a couple of areas where assignment is required for the contained type T (e.g. pimpl_ptr::operator=) and "operator==" is required for the contained type in at least two places. I'm not sure what you mean here. The examples given should very clearly show what is required from the pImpl class.
5. Looking at the code, the pimpl_ptr copy constructor is performing assignment in the ctor body ... how is the deep copy being performed? I don't see deep copy semantics in either the "base" class or the policy classes. Does this affect the requirements of the contained type T? Deep copying is performed by converting the pImpl pointer to a reference, and copying.
6. If a pimpl_ptr is default constructed with a manual_creation policy, how is the impl object created / passed in at a later time? By using 'set'.
7. One of the issues with policy based template classes is compatibility / conversion between types with different policies. In this library, pimpl_ptr<Foo> and pimpl_ptr<Foo, lazy_creation> are different types, and it's not clear to me how / if they can interact with each other. Since all policies implement the 'get' function, all relevant operations can be performed.
8. Swap is not swapping policies, only the underlying impl pointer (unless I'm confused). That's correct. You cannot swap policies. That's how C++ works (unless we recreate each pimpl_ptr, etc. but that would require more work than perhaps desired by the user).
Hope this helps, It helped a lot. Thank you.
Regards, Asger
participants (5)
-
Andreas Huber -
Asger Mangaard -
Cliff Green -
David Abrahams -
Jeff Garland