Re: [boost] [pimpl] Some comments

Subject: Re: [boost] [pimpl] Some comments
From: Vladimir Batov (Vladimir.Batov_at_[hidden])
Date: 2014-06-05 21:51:18

Andrzej, Thank you for your quick reply. :-) ... I did not even know 
Glen already advertised the upcoming review. Please find my replies 
below. (I snipped some for brevity. My apologies)

V.

On 06/06/2014 07:21 AM, Andrzej Krzemienski wrote:
> ...
>
> [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.)

Well, I feel you mix up two things here: pre-build analysis and the 
build as such. The number of included headers has impact on the first 
phase -- pre-build analysis -- as 'make' has to check those 
included-files timestamps. I do not believe it's such an issue as I do 
not expect to notice much/any performance differences from checking 1 of 
10 timestamps. So, if you have pimpl.hpp included with a few other Boost 
headers thrown into the mix, then 'yes' it does increase the time of 
pre-build analysis... which is negligible compared with the time of the 
actual build. Given those mentioned Boost headers do not change, they do 
not trigger re-build. That said, when re-build is triggered by other 
code, those headers to need to be compiled and that takes time.

Now we are getting to the build itself. The thing is that build performs 
the fastest when it, well, does not build. And that's what Pimpl allows 
you to achieve. So, in short, if you deploy the Pimpl idiom, then you 
manage to avoid re-builds. The time/performance difference between your 
solution and mine will only be in pre-build-analysis time... which I 
claim to be fairly minimal if noticeable at all.

As for doing Pimpl yourself vs. a library solution, then it's a 
different matter. I'll take the latter any day... and will make sure 
people on my project do the same... if they want their code to pass 
through review. :-)

> [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.

We might be developing for different industries. I've been working for 
rail and air for the last 15 years and there shared data are everywhere. 
Rail network topology is only one, aircraft data, airline schedules are 
all shared by many threads. So, my focus on shared_ptr-based 
pimpl::pointer_semantics is clearly biased.

> It looks from the docs that 'pointer_semantics' implements something like a
> flyweight.

Yes, it is a variation of the Flyweight idiom... as shared_ptr and raw 
pointers are... Although I personally do not like the name as I find it 
confusing. In GoF it is the shared object (big and expensive to copy) 
which is called "flyweight object". To it is the proxies (shared_ptr, 
raw pointers) which look like "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.

Well, I certainly agree that value-semantics classes are quite different 
from pointer-semantics classes... and it is expected that user knows 
what he/she is using. Pimpl tries to help by making it clear in the 
declaration:

class Book : public pimpl<Book>::pointer_semantics
class Book : public pimpl<Book>::value_semantics

To me "doesn't look like a pointer" sounds like a weak argument because 
there are many things in C++ which do not look like a pointer:

1) non-const reference;
2) typedef shared_ptr<foo> my_foo;
3) many classes in X Window System (like Widget) are actually pointers.

What I am driving at is that the user ultimately needs to know what 
class and its behavior actually are to use it effectively.

That said, I do acknowledge a potential for mis-use. In my code I 
address it with "const". That is, say, shared_ptr<foo const> is passed 
around and has to be copied to shared_ptr<foo> before being modified.

> [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.

op==() as a member works well for object of he same type, like pimpl1 == 
pimpl2... and for pimpls I believe it's all what's needed. I might be 
wrong though (just have not had practical applications of different 
sort). I'll look into it though.

> [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?

I could not remember what I based those statements on. :-( I started 
researching and writing a reply but got totally tangled up with all 
those exception-guarantee complexities... Got totally confused... 
removed the part.

> ...
> While I find the value_semantics interface useful, I am strongly against
> even offering the pointer_semantics interface.

Hmm, that "strongly against" indicates some conviction and I think Gavin 
echoed that view in a later post. I'll probably need to revisit my lib 
designs as I am using it a lot. For example, right now I read a DB, 
cache it. Then everyone has read-only access to records via pimpl-like 
shared_ptr-based facility. It's all kosher as read/access requests 
mutexed and fast. Where is the problem? I seem to be missing something big.