Re: [boost] [pimpl] Some comments

Subject: Re: [boost] [pimpl] Some comments
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2014-06-07 08:05:39

2014-06-06 3:51 GMT+02:00 Vladimir Batov <Vladimir.Batov_at_[hidden]>:

> Andrzej, Thank you for your quick reply. :-) ... I did not even know Glen
> already advertised the upcoming review.

This looks like a really good idea. A pre-review notice, which gives us an
opportunity to clear up many thing before even the review is started.

>
> 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. :-)

You convince me. What you and others in this thread say indicate that I do
not fully understand the benefit of pimpl idiom.

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

While I agree with this last statement alone, ("you must know the types you
use"), I am uneasy with the conclusion about pimpl::pointer_semantics. I
realize that so far the only thing I expressed is my *preference*, which
may be too little for a discussion, so I will try to come up with more
objective arguments (and see if there are any, or if this is just a
personal taste).

I still claim that pimpl::pointer_semantics breaches value semantics
expectations in an unprecedented way. Both non-const refs and shared_ptr,
and raw pointer and normal value semantic classes have one property in
common: Their operator== reflects their *salient attributes*. Sorry for a
fancy term. I borrowed it from John Lakos:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2479.pdf,
https://github.com/boostcon/cppnow_presentations_2014/blob/master/files/accu2015.140518.pdf?raw=true

Let me illustrate this. When I have two objects of type 'book` with the
following members for accessing key components of the value

Book book1 = make_book();
book1.title()
book1.author();

I expect that expression (book1.title() == book2.title() && book1.author()
== book2.author()) is equivalent to (book1 == book2). Note that this
expectation holds for references. It also holds for smart (and dumb)
pointers:

std::shared_ptr<Book> book1 = make_book();

book1 does not have member title(). The following is invalid:

book1.title(); // INVALID

the indirection operator -> or * already tells me: these remote parts are
not part of book1 value. The value of the pointer is the address it stores.
It is consistent in the sense that its operator==  only compares its value.

In contrast, pimpl::value_semantics give me direct (no operator* or ->)
access to title() and author(), and a surprising behaviour:
Book book1 = make_book();
Book book2 = make_book();

book1.title() == book2.title() && book1.author() == book2.author() is true,
but book1 == book2 is false, because it so happens that they refer to two
distinct remote objects. This is a precedent in STD and boost (unwanted, if
you asked my opinion).

I do not know what Widget in X Window System does, but I know that not
every library in C++ is worth following, and its design worth promoting in
Boost.

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.

This is a safe subset of shared_ptr usages.

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

This works in most of the cases, but consider:

class AudioBook // no inheritance
{
  // some stuff
  operator Book() conts; // convert to your Book
  // no opereator== because we want to use yours
};

AudioBook a;
Book b;
b == a; // works
a == b; // ERROR

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

if you are using it like this:
shared_ptr<const DB> make_DB();

Then it is fine I guess, although I would rather create a non-copyable
value object in one place and pass around references to it (may not be
always possible, I guess). But it is superior to pimpl::valu_semantics in
two ways:

   1. It does not allow mutability
   2. It has value semantics (a pointer --even smart-- is a value: it
   represents an index in a big array -- free store memory).

I hope I managed to communicate my thoughts more clearly.

Regards,
&rzej