Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Sean Chittenden (sean_at_[hidden])
Date: 2011-06-01 12:02:45

>> I've been watching this discussion for a while (with vested
>> interest), and want to use a few examples to shore up some of
>> the discussion (and possibly move it off list so that things
>> can proceed in a constructive manner). The concern is not that
>> Pimpl is implementing an opaque pointer, it's the miscellaneous
>> copy constructors, constructors, equality or whatever other
>> misc "extras" that come along from using Pimpl, right?
> 
> Not quite.  Copy constructors and copy assignment, for example, are appropriate.  They allow the library to manage copies of the implementation object.  Problems arise, for example, from the addition of null(),operator ->(), and operator *().

This is less rhetorical than I'd like, but what's the right amount of baggage that a Pimpl implementation can bring along with it? On one side of the spectrum you have an austere implementation that is literally an opaque pointer with its relevant compilation glue ("the one true/pure pimpl"). On the other side of the spectrum you've got something that overloads functions and operators and makes a pimpl implementation easier to use, ala the Pimpl library. For example:

https://github.com/sean-/Boost.Examples/blob/master/pimpl/boost/pimpl/pimpl.hpp#L134

Not strictly necessary, but I understand why it's there and why it would be wanted in a Pimpl implementation. I also understand the puritanical argument for why you wouldn't want it.

> reset(), which may be protected rather than public (I don't recall), is not as much of a problem, but my suggestion for that was that since the constructors allocate the implementation object, "resetting" the implementation should likewise not involve allocation.  For that, I suggested a construct() member function template set, paralleling the constructors that implement "perfect forwarding."  A constructor and a reset() that take a deleter would be appropriate for cases in which allocation is not desired.  That design would be symmetrical.

Got it, ok.

>>> class Book : public pimpl< Book >::pointer_semantics { };
>>> // Or is the interface different because the ABI is
>>> // different?
>>> class Book {
>>>  class* impl_;
>>> };
> 
> It's different because using inheritance and public member functions, pimpl<Book>::whatever has augmented Book's interface.

Correct/agreed.

>> With as (in?)efficient compilers sucking up gigs of RAM and
>> frequently crashing during compilation[1], having a friendly
>> mechanism to hide linkage and implementations behind opaque
>> pointers is useful beyond externally exposed APIs in libraries.
>> The tedium of doing this frequently makes the Pimpl library an
>> easy win for many developers (though possibly not all of them
>> and not in all situations). As a Pimpl consumer for over a
>> year, I'm trying to concretely demonstrate where the possible
>> pitfalls could be lurking with this implementation. Thanks in
>> advance. -sc
> 
> I have no problem with using the Pimpl Idiom.  Just this past week I was able to advance some application testing readily because we used it in a couple of classes involved in an application's disorderly shutdown due to misconfiguration.  My concern is that the Pimpl Library seems to have pushed past the idiom into something else, which is surprising to me and, clearly, to others.  I should also be clear that I'm not against what the Pimpl Library has done, in terms of augmenting the derivate's interface via public inheritance, but that doing so should be in a class template and a library of a different name.

That feedback makes sense, thanks Robert.

I think the unease is coming from the Pimpl library implementing helpers and extending beyond only providing an opaque pointer. TBH, before using Pimpl, I'd implemented a subset of the helper functions in my own home-grown pimpl library so seeing them standardized never struck me as objectionable (the possibility of running in to name clashes, not withstanding). 

In fact, my search for Boost.Pimpl began because I missed adding a non-const* ->() to my implementation. After wading through pages of gcc error messages (naturally this happened in the middle of a asio/bind/wrap call) I started looking for a reference implementation that had some of this spelled out since I didn't think I should be spending my time reimplementing the wheel. I don't have an answer for what's "right" on the spectrum of austere vs "featureful", but the featureful side of Pimpl has clear merit.

I'll move the rest of my comments over to the review tool. -sc

http://demo.smartbear.com/boost/go?page=ReviewDisplay&reviewid=5

--
Sean Chittenden
sean_at_[hidden]