Artyom Beilis writes: ... I'm sorry but I have never called anybody "an illiterate moron",
That was my "artistic interpretation" of the words you were quite happy to use. BOOST_ALMOST_ASSERT( "an illiterate moron" == "without knowledge of the problem domain");
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.
Indeed, this is a forum where everyone is entitled to his opinion. However, things that you so unfortunately sprinkle over your posts sound more than just your opinion. More like a patronizing lecture or a scolding of a naughty child. Are you sure you have the credentials to tell us all what we (I quote) "should not care about" or "almost never want". Are you sure you have the right to speak for the "average programmers", to decide what is "overcomplicated", "hard to read" and "makes more troubles" for them? Are you sure you are well-qualified to say what is "useless" and what "90% of pimpl use cases" are? ... And thanks, man, for not calling me a moron "EVEN thou" you stand behind your opinion. :-)
... 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.
To begin with you do not "bring up all the problems". You bring up the details/issues that you *perceive* to be potential problems. Your perception (as everyone else's mine obviously included) might turn out to be spot-on or wrong (can you imagine?) for one reason or another. Secondly, it's not a review. There is no Review Manager, there is no time frame, there is no outcome/resolution. Rob Stewart needed to test that new Collaborator tool. So, he thinly-veiled that need into a "pre-review" offer and lured me into that trap for seemingly no particular gain for me.
… 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.
You have to make up your mind. Isn't it that "shared_ptr as pointer model is Wrong. You almost never want to have shared object semantics for pimplized object"? Does your alternative implementation do it all better and provides op==() but not the "implementation detail" which I am not sure what you are referring to? "My dear" Pimpl gives us *all* a choice -- I can use shared_ptr-based deployment with all the perks I value and you can use value_ptr-based private-inheritance deployment and have full control what you expose to the user.
My pimpl usage patterns are different. They happen to be overwhelmingly with pointer semantics.
I'm not telling that this is a valid use case. Of course it is, but it is only one of them.
Again, the majority of other use-cases that are important for your deployment are covered by the value-semantics Pimpl (or can be covered by new policy classes). Some of the use-cases that you describe as vital for Pimpl deployment I strongly disagree with. Like struct { string title_; string author_; struct impl; smart_ptr<impl> impl_; } Such a half-Pimpl half-not monster is not a Pimpl and feels wrong on so many levels. Consider yourself lucky you are not working on our projects and do not have to pass our code review. :-)
You can successfully deploy this use case with using shared_ptr as member that represents opaque pointer.
Here we go again. I am sure you know better what I can and what I cannot... even though you do not deploy shared_ptr pimpls. If I could, then I would. After doing it over and over again I just noticed repeating the same stuff... which I packaged up in "my dear" Pimpl to avoid repeating it. Is it a lot? Nop. Is it useful to deal with a thought-through idiom implementation rather that repeating that by hand? I think so. You might disagree.
2. The real relational operator should use implementation class and it does not provided automatically.
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.
Wrong again. "My dear" Pimpl *delegates* that functionality to the underlying policies. Therefore, those relational operators are provided for shared_ptr-based deployment as it makes sense and shared_ptr provides those. For value_ptr-based implementation there are no default relational ops.
But as I noticed before private inheritance would remove some other things that in some cases you may use.
Indeed, I need/use the functionality Pimpl provides. I do not privately inherit. You insist that that's too much exposure. So, I say, inherit privately if you like, and do the rest by hand. And I am sure you know how to selectively enable otherwise disabled base-class functionality: class derived : class base { void foo() { base::foo(); } }
... Library does not even provide an option to have a pimpl as member that is why I think it is inflexible.
Clearly "my dear" Pimpl does not do it *all* for you. It does not *even* provide that option above. ;-) There is something that you still have to do by yourself or you deploy plethora of other smart pointers. The library addresses a certain well-defined concept/issue. It is about constructing a Pimpl class easily, safely and painlessly. How you deploy that Pimpl later is up to you. If you want to make it a member of another class, be my guest. You might have some very special/specific use-cases. Those use-cases are not necessarily to be covered by Pimpl. Maybe some other smart pointer is in order. Horses for courses.
I does not do what **I** personally expect from the library and many others.
Of course, it does what you want. Value_prt-based, private inheritance. It just happens to do it somewhat differently. Something you refuse to admit/accept/acknowledge. ... And ***I*** like the "many others" part. :-)
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.
Well, I do not know how to reply to this "argument". That would probably convince me ;-) ... but then I remembered that quite a few people in *my* vicinity are happy to use it and do not find it "over-complicated". Then I remember that after the original article was published Herb Sutter was kind enough to send me an email appreciating it. So, now I have to choose -- Herb and my peers or some misterious guy you talked to? Hmm, so hard to choose. ;-) I am not even sure why you insist on bringing that silly "over-complicated" word here. I am sure it's not complicated for you or the user in general for that matter. If feels like you are not playing fair -- when it suits you, you play "dumb" and bring that "over-complication" as an argument; when it suits you better, you play "smart" and bring some twisted not-even-Pimpl use-case.
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.
LOL. I thought we had turns and you already had yours.
- No support of custom implementation classes. Consider I do not understand what you mean by “custom implementation classes”.
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...
That's it? Is it *really* all? I am lost for words, really. Well, pimpl<foo>::implementation is still *your* class. You implement it. Indeed as an old and boring person I insist that that implementation is part of the Pimpl concept and, therefore, should be coded/presented as such (for easier understanding, self-documentation, maintenance). You, instead, insist on some other name... any name... but not pimpl<foo>::implementation... is there something inherently objectionable about pimpl<foo>::implementation or it is just a rebellion against someone "telling" you what to do?
No need for those “place holders”.
I think that I myself and Qt would disagree. Placeholders are critical for keeping stable ABI.
I think that I myself and Qt would disagree with your disageement. I deploy Pimpl with Qt a *lot* as we extend Qt widgets and adapt Qt for our specifics (it does not play nice in multi-threaded environments). No-one denies the importance of stable ABI. It's just other people might find that place-holders idea bizarre and addressable some other more conventional way. I've been using "my dear" Pimpl for that very purpose. The Pimpl's property you seemingly deny.
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.
If you want to extend an existing class which size you cannot change, then pimpl or no-pimpl, place-holder or not, you are stuffed -- you cannot extend the class. If you can change the size, then instead of extending struct book { string title_; string author_; } with struct book { string title_; string author_; struct added_data; smart_ptr<added_data> added_; } I'd do struct book { struct added_data; smart_ptr<added_data> impl_; } where all data including title_ and author_ go into that impl_ which you then extend as you like.
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.
Yes, I think I am done here as well. I can't wait for the real review to get it over with one way or the other. V.