2014-06-06 3:51 GMT+02:00 Vladimir Batov
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/accu... 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