[boost] [pimpl] Some comments

Subject: [boost] [pimpl] Some comments
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2014-06-05 17:21:04

Hi Vladimir,
My congratulations on the success of Boost.Convert. But I can see you will
not have much time to celebrate it, next library waiting in the queue.

I wanted to share some of my thoughts on Boost.Pimpl library. This is by no
means a review. I am not sure if I will have time to do a decent one. I
only skipped through the documentation so my understanding is superficial,
but I noticed some things that raise my immediate objection.

Boost.Pimpl has been around for a while now, and I remember I was using it
at some point in my work, although it didn't have the nice documentation
format at that time.

[1] The main point in using pimpl idiom (at least my) is to avoid header
inclusion, which may grow compilation time too much (at the expense of
certain run-time cost). But in order to use Boost.Pimpl I do need to
include a header file, which in turn includes more header files. This is
arguably less headers that in normal non-pimpl implementation, but it is
more than if I used a raw pointer. And this is what I finally did in my
work: switched to raw pointers for implementing piml in order to get the
build perform faster. (I didn't measure the improvement though.)

[2] Boost.Pimpl is good for everyone: people who like value semantics get
the value semantic interface, and people who like shated_ptr's get
shared_ptr semantics. Call me selfish or biased, but I believe using
shared_ptr's should be banned (at least in 90% of the cases where it is
currently being used), and the fact that you advertise it first in the docs
makes me uneasy.

It looks from the docs that 'pointer_semantics' implements something like a
flyweight. This is so against value semantics. One of the expectations of
types returned by value is that changing the original does no affect the
copy:

T a = c;
T b = c;
zap(a);
assert(b==c && a!=b);

Although the author of class Book knows that it is implemented with
pointer_semantics, the users of the class are likely not to know it, and
since Book doesn't look like a pointer, they will be expecting value
semantics, and will be severely punished.

[3] The docs advertize the definition of operator== as a member function.
This is against a typical recommendation that it should be a free function
in order to avoid asymmetrical behavior in certain cases. Does your library
allow the definition of a non-member operator==? If so, I would encourage
you to reflect that in the docs.

[4] This page:
http://yet-another-user.github.io/boost.pimpl/the_boost_pimpl_library/pimpl__null___and_strongly_exception_safe_constructors.html
says "The advantage of the 'built-in' *Book::null()* [...] is that it makes
possible an implementation of strongly exception-safe constructors, the
copy constructor" There seems to be something wrong with this statement.
How can a strong (commit-or-rollback) guarantee apply to constructors? If
an exception is thrown from constructor the object is never created or
accessible, so no-one cares about its state (as long as it doesn't leak).
Or did you mean the no-throw guarantee? But the latter looks like an anti
pattern. What is  the value in concealing the exception (and information it
carries) and instead produce null objects, which cannot be used?

While I find the value_semantics interface useful, I am strongly against
even offering the pointer_semantics interface. I am not sure if I have
succeeded in explaining why.

Regards,
&rzej