From: "Asger Mangaard" <tmb@tmbproductions.com>
Pavel Vozenilek wrote ...
Closing bracket for namespace should nopt have ";" but should have comment like // namespace pimpl_policies
It's corrected.
"Should" is a strong word there. I dislike such comments because they too easily get out of sync with the start of the block. Some like that style, some don't.
----------------
operator=() does shallow copy of dynamically allocated value. Is this really what ypu want?
It's corrected.
I just downloaded the library and it still does a shallow copy. If you intend something else, then you need to rethink how you handle copying. ___________________________________ I have some of my own comments now. * Why the pimpl_policies namespace? I think you should have a boost::pimpls namespace. In that you can have the pimpl class and the supplied policies. * The base class should be in namespace detail. The name suggests that this type isn't one clients of the library should be using. However, the behavior of the class suggests that you really want it to serve two purposes (also evidenced by its being the default pimpl policy type). I suggest a new type, derived from base, called, say, "default_construction" that can be the default policy. That class can construct an instance in its default constructor and provide the get() member function you currently have in base. Once you do those things, then base can have a constructor taking a raw pointer to the impl object and you can eliminate the get() member function. At that point, base will have only one purpose instead of the two it now has. * Why is "PimplType" not just "T?" The whole point of the template is to house a pimpl type, so the added verbosity doesn't seem helpful. * base::get() error handling should be configurable or nothing. Throwing an exception isn't always the appropriate action. Some would prefer getting a segfault and a core dump, for example. Either omit the test of m_pPimpl or control it via another policy. * You m_pPimpl data member name is quite odd. The reason this is the "pimpl" idiom is because of the common "p" prefix for pointers to an implementation object. That leads to "pimpl_," "m_pImpl," "_pimpl," and other variations, depending upon your naming scheme. Using both the "p" prefix and the "P" in the data member name is redundant. My preference is "pimpl_," but "m_pImpl" is probably yours. * boost::pimpl_policies::base::base() misuses the new operator. "new PimplType()" is incorrect. You want "new PimplType." * boost::pimpl_policies::base::~base() should be non-virtual. No one should be creating polymorhpic instances of the pimpl. * Your explanation of the mutability of m_pPimpl is redundant. That's the reason mutable exists. You might want to be more specific as to the function that uses it or just remove the comment. * Since lazy_creation and manual_creation derive from base, you'll cause warnings about hiding base::get(). base should not have a get() member function. The derived policies should have to implement get(). * I'm not sure whether manual_creation should be a separate policy. Why not have the default policy have a constructor taking a raw pointer, defaulted to a null pointer, plus provide a set() member function? You'd need to implement set() just as it is in manual_creation now. namespace boost { namespace pimpls { template <typename T> struct default_ : base<T> { default_(T * pimpl_i = 0) : pimpl_(pimpl_i) { } T & get() const { return *pimpl_; } void set(T * pimpl_i) { delete pimpl_; pimpl_ = pimpl_i; } }; } } Thus, default_ does what your base and manual_creation policies do in one simple policy. * Don't define empty destructors. They are noise in the code and can actually be a hindrance to optimal code generation. * pimpl's member functions should be inlined. They are all trivial, so you should make them inline. * Consider "rhs" or "other" instead of "comparer." This is simply a stylistic suggestion which isn't terribly important, but your use of "comparer" stood out. First, what you've called "comparer" isn't comparing. Second, it's a rather long name for something so straightforward. -- Rob Stewart stewart@sig.com Software Engineer http://www.sig.com Susquehanna International Group, LLP using std::disclaimer;