----- Original Message ----
From: Vladimir Batov <vb.mail.247@gmail.com> To: boost@lists.boost.org Sent: Sat, May 28, 2011 9:03:53 AM Subject: Re: [boost] [pimpl] Mini Review
"Artyom Beilis" wrote ... Hello Vladimir, I apologize from all my heart. I'm sorry if my tone was rude or disrespectful. I want to do the discussion in positive way and I do not want to be disrespectful to anybody. I'm sorry if it feels wrong.
Artyom,
I honestly believe that offending anybody is the last thing on your mind and your primary/only objective is of technical nature. However, you'll probably agree that accusing someone you-know-nothing-about of being "without knowledge of the problem domain" oversteps that technical line and looks more like a unwarranted personal attack. What's worse is that after such a personal "treatment" the person you are trying to communicate with gets understandably defensive and is not open to the conversation. So, your otherwise useful other suggestions/comments are not being heard.
From your CV I read you hit 30yo this year. Wonderful age. It feels like the world is your oyster. At 30 one still has not lost the energy and enthusiasm (and illusions) of 20yo and at the same has got the knowledge and the means to have things done. It feels like the world and its forces are well understood; and you know the *right* way; you know the destination; you know how to get there.
So, anyone doing things differently is an illiterate moron "without knowledge of the problem domain".
I'm sorry but I have never called anybody "an illiterate moron", even thou I fully stand behind the fact that current Pimpl library covers very partial set of use cases. That is why feel it does not fit the problem domain. This is my opinion, it is different from yours but this is what reviews are for - provide an opinion that may differ from the opinion of the library author.
I am unfortunately considerably older (and that sucks let me tell you). I had more time to learn and as a wise saying goes – the more I learn the more I realize how little I know. So, with time I might ultimately agree with you that I am an illiterate moron "without knowledge of the problem domain"… but I am not there yet. So, let’s talk Pimpl that you know so well and so eager to teach
us
all how to use it properly (a little tease won’t hurt, will it?). ;-)
I'm not trying to teach anybody but I have an obligation as a reviewer to bring up all the problems I see with this library.
… Pimpl based should not expose implementation details.
Are you saying “my” Pimpl exposes the implementation details? Have you had a chance to read the original article? “My” Pimpl does expose the
implementation
details… but only to the implementer of those “details”, not to the user.
Only if you inherit from pimpl<Foo>::xyz_semantics privately but then... You do not get for example operator== that you may actually want in case of pointer_semantics but not want to expose where the underling object is null or not.
Pimpl class and normal class should not look different from user point of view.
I probably do not understand what you are trying to say. As I read/interpret it, it’s plain wrong. Pimpl tries to solve exactly what is wrong with the
“normal”
class – tight coupling of interface and implementation. So, Pimpl and “normal” classes cannot possibly look the same. If by “the same” you mean the interface, then yes, in that respect Pimpl is no different from “normal” classes. In fact, it *is* a normal class.
It is correct design approach - decoupling an implementation from the interface. Exposing implementation details in Pimpl is bad design
Exposing implementation details in Pimpl is not just bad design... it’s just not the Pimpl idiom to begin with. I feel you might like to consider having another look at “my dear” Pimpl and reading the original article to realize that separation of the implementation and the interface are as dear to my heart as it is to you.
Two points:
1. pimpl object should be compared by value as the fact that there is a some
pointer behind is implementation detail so current approach is
incorrect
from my point of view
Well, what ceremonies between friends, right? So, I’ll give it to you straight, my friend. That’s rubbish. The correct answer is “it depends”. Indeed, as I read from your other Pimpl-related posts, your usage patterns are overwhelmingly/exclusively with pimpls with value semantics. My pimpl usage patterns are different (can you imagine? ;-) ). They happen to be overwhelmingly with pointer semantics. I’ll give you a simplified example (so do not jump in with “you all are doing it all wrong!”). We read database records and we cache them. We cache them as shared_ptr-based Pimpls and we pass them around. So, everyone using a particular record happens to share the same underlying record data. If that record changes in the database, we discard the old version, cache the new version, we send out update notifications. So, if one happens to keep a Pimpl to that record, then he will have the original (now old, which he kept) and the new record. We deploy that mechanism in several projects doing railway traffic monitoring (of the whole city), airline scheduling and rostering, etc. So, it’s not exactly chicken feed. Your projects are probably of different nature and you probably do not need features I need. That’s why “my” Pimpl provided pointer-semantics to keep me happy and value-semantics to keep you happy… although for unknown reasons you are adamant in not noticing that “my” Pimpl offers the functionality you are after.
I'm not telling that this is a valid use case. Of course it is, but it is only one of them. You can successfully deploy this use case with using shared_ptr as member that represents opaque pointer.
2. The real relational operator should use implementation class and it does not provided automatically.
If I was talking to a friend (after two glasses of vodka), I’d probably say that such a blunt statement is naive and immature. Given I prefer beer and we are only getting to know each other ;-), I’ll stop short saying I am not sure I can agree as I am under impression that the correct answer is “it depends”.
Yes, it depends, but this is what your library suggests by default.
For value-semantics Pimpls you are probably correct for the majority of cases. However, there is a bigger world out there which does not play by or follow the rules you set in your nick of the woods.
safebool operator; It is implementation detail, it should not be exposed to pimpl user. pimpl::null (and pule-eee-ase do not tell me that *I* do not need that); It is implementation detail, it should not be exposed to pimpl user.
I beg your pardon?! Since when safebool and pimpl::null have become “implementation detail” and “should not be exposed to pimpl user”. The Pimpl idiom allows the user to have these perks that raw pointers provide (like no data behind the pointer or NULL pointer). Denying that to the user is nothing short of robbery. Even many value classes provide that functionality. Like many classes construct an invalid object with a default constructor. Still, if you want to deny it to your users, use private inheritance. Just do not deny it to the whole world.
the proper deployment of the incomplete-type management technique (for
But as I noticed before private inheritance would remove some other things that in some cases you may use. the
destructors to always work correctly).
This is the only actual point I like in this library but I think it should be solved in scope of some smart pimpl pointer and not in the approach of this library.
Aha, you insist on the design like
struct my_class { interface; private: pimpl_ptr implementation_; };
I do not see it much different from the *tactical* inheritance like
struct my_class : pimpl_ptr { interface; };
Which I find cleaner from user perspectives and often more powerful. So, if do not like safebool and relational operators – use private inheritance. Just do not deny that functionality to others and do not tell them that they do not need/want that.
Library does not even provide an option to have a pimpl as member that is why I think it is inflexible. I does not do what **I** personally expect from the library and many others. For the record I had once a discussion about pimpl with somebody that had same opinion - it is over-complicates simple things that should not be this way.
This is a list of problems I see in this library:
- Pimplized object should not derive from some other objects - wrong approach and design (IMHO)
Here we go again. :-) Look, Artyom, I cannot even be angry with you anymore. You are like a child, really – you stomp your foot and say – wrong design.
Please don't go personal this is not apropriate.
End of story. Just accept it – if you wear shoes size 44, then it does not make any other sizes wrong. Other people *need* different sizes and styles and colors.
This is MY opinion whether you like it or not thats it.
- No support of non-copyable implementations (with value_semantics)
That’s incorrect. If one does not implement copy constructor for the value-semantics Pimpl, then it’s not copyable. More so, kick in boost::noncopyable at any time if you need it.
I've compiled a small code where implementation class was non-copyable with value_semantics... It didn't compile.
- No support of custom implementation classes. Consider
clss unicode_string { private: smart_ptr<icu::UnicodeString> pimpl_; };
I do not understand what you mean by “custom implementation classes”. All classes derived from Pimpl are “custom implementation classes”. They are just not implemented as you like it.
I'll explain. Your library restricts that implementation class to be: class foo : public pimpl<foo>::value_semantics { ... }; template<> class pimpl<foo>::implementation { ... } Sometimes I want it to be different things. My own classes. Not pimpl<foo>::implementation...
- Better support of place holders (empty implementation object for future use) is required.
[snip]
Pimpl supports modifications naturally by separating interface from implementation. So, as long as interface stays, the implementation can grow/change as you please.
No need for those “place holders”.
I think that I myself and Qt would disagree. Placeholders are critical for keeping stable ABI. That is one of the very important use cases of Pimpl in C++. Sometimes you want just extensible class and without such placeholder you can't add members to object without breaking ABI. Bottom line: 1. I don't think Boost.Pimpl flexible enough to be part of Boost 2. I don't think Boost.Pimpl's design approach is good enough to be a part of Boost. Nothing personal, just a review that expresses my opinion and my experience in this area. Best Regards, Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/