Re: [boost] [pimpl] Mini Review
----- "Artyom Beilis" <artyomtnk@yahoo.com> a écrit :
Hello All,
Because a pre-review runs this days in collaborator tool I still want to provide some feedback on this library I was looking on long time ago.
- What is your evaluation of the design?
--------------------------------------------- This is the major problem of the library: It tries to do simple things overcomplicated. ---------------------------------------------
Take as an example:
struct Book : public pimpl<Book>::value_semantics { ... };
Is an attempt to do things in "fancy" way where it is not needed.
struct Book { public: private: struct implementation; smart_ptr<implementation> pimpl_; };
Is much clearer.
Your code would rather simulate pimpl<>::pointer_semantics. And you do not mention the boilerplate code generated.
The proposed solution is overcomplicated hard to read and in general makes more troubles then helps, makes the code less readable by average programmers.
------------------------------------ shared_ptr as pointer model is Wrong ------------------------------------
(snip lots of smart_ptr related lines) Unless you want pointer_semancics ... As for value_semantics, shared_ptr is not used.
2. clone_ptr - pointer that clones underlying object on its copy. i.e. something like
copy_ptr(copy_ptr const &other) : p_(other->clone()) {}
Precisely what seems to be used in the library for value_semantics.
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
An important advantage as many developpers get trapped here. And it is "only" a warning to do so ...
Spare Pointer use case ------------------------
Another important use case in the case when pimpl is used to handle ABI stability and in general the object has spare "pimpl" or d-pointer for future uses.
Consider:
struct book { public: book(); book(book const &); ~book(book const &); book const &operator=(book const &); std::string title() const; std::string author() const; void title(std::string const &); void author(std::string const &); private: std::string author_; std::string title_; struct data; copy_ptr<data> d; };
Now in book.cpp we define data as struct book::data {};
And with Pimpl, you would put author_ and title_ in book::data. I failed to see the point here. You seem to say that Pimpl cannot be used for ABI stability, which seems wrong to me. Regards, Ivan.
From: Ivan Le Lann <ivan.lelann@free.fr>
----- "Artyom Beilis" <artyomtnk@yahoo.com> a écrit :
Take as an example:
struct Book : public pimpl<Book>::value_semantics { ... };
Is an attempt to do things in "fancy" way where it is not needed.
struct Book { public: private: struct implementation; smart_ptr<implementation> pimpl_; };
Is much clearer.
Your code would rather simulate pimpl<>::pointer_semantics. And you do not mention the boilerplate code generated.
Boost pimpl does not save anything in terms of boilerplate code. What is the difference between: class foo { public: foo(int x = 0); int v() const; void v(int x); private: struct implementation; boost::shared_ptr<implementation> pimpl_; }; struct foo::implementation { public: implementation(int x = 0) : v(x) {} int v; }; foo::foo(int x) : pimpl_(new implementation(x)) { } int foo::v() const { return pimpl_->v; } void foo::v(int v) { pimpl_->v=v; } And class foo : public pimpl<foo>::pointer_semantics { public: foo(int x = 0); int v() const ; void v(int x); }; template<> struct pimpl<foo>::implementation { public: implementation(int x = 0) : v(x) {} int v; }; foo::foo(int val) : base(val) { } int foo::v() const { return (**this).v; } void foo::v(int v) { (**this).v=v; } Not too much, except that first seems to be much more flexible.
(snip lots of smart_ptr related lines)
Unless you want pointer_semancics ... As for value_semantics, shared_ptr is not used.
2. clone_ptr - pointer that clones underlying object on its copy. i.e. something like
copy_ptr(copy_ptr const &other) : p_(other->clone()) {}
Precisely what seems to be used in the library for value_semantics.
No, hold_ptr - for non-copyable objects, clone_ptr for cloneable objects. Copy ptr is fine. It does not provide too much semantics. Also it exposes some details like "operator bool()" to places that not belong to. You should not care about whether the internal pointer null or not. It is an implementation detail!
The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor.
An important advantage as many developpers get trapped here. And it is "only" a warning to do so ...
So create hold_ptr/copy_ptr/clone_ptr with information about how to destroy, copy and clone in the same way shared_ptr does. In any case just adding ~foo(); in h and foo::~foo() {} in cpp solves problem. Not justified.
Spare Pointer use case ------------------------
Another important use case in the case when pimpl is used to handle ABI stability and in general the object has spare "pimpl" or d-pointer for future uses.
Consider:
struct book { public: book(); book(book const &); ~book(book const &); book const &operator=(book const &); std::string title() const; std::string author() const; void title(std::string const &); void author(std::string const &); private: std::string author_; std::string title_; struct data; copy_ptr<data> d; };
Now in book.cpp we define data as struct book::data {};
And with Pimpl, you would put author_ and title_ in book::data. I failed to see the point here. You seem to say that Pimpl cannot be used for ABI stability, which seems wrong to me.
I explain. I want to create a class that I may add members in future to it. So I just put there an opaque pointer that would be used in future. This is typical use case in many-many places. I don't want to **derive** my class from some fancy other class to provide an opaque pointer for future uses. It is total overkill for something that is not used at all. So basically I don't want to have title_ and author_ to be members of data as I don't want to use data right now. To make it more clean, consider them numeric values like int with inline accessors that are very fast and efficient and do not even require memory allocation to copy an object from one place to other. Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
Artyom Beilis <artyomtnk <at> yahoo.com> writes: ...
Well, these reviews are clearly "fun". Is it that I am getting old and touchy or these new kids on the block are getting vicious or they are learning C++ before they learn "please", "thank you" and "with all due respect"? Artyom, if you want to have a civilized conversation/discussion or at the very least to maintain any *pretense* of a civilized conversation/discussion, then you probably might like to have the other party *willing* to talk to you. If so, then *please* try to refrain from statements like "without a knowledge of the problem domain", "you need", "you want" and similar kind. I suspect quite a few people might find such style as rude, objectionable and arrogant. As it is now apart from this short email I suspect I won't be replying to your "comments" related to the Pimpl library.
Boost pimpl does not save anything in terms of boilerplate code.
Well, that's most certainly complete and utter bullshit (is it the language our new developers speak/understand these days?)
What is the difference between:
... I snipped the actual manually-crafted and Pimpl-based code. The difference is that the Pimpl-based version provides the following functionality which your "flexible" alternative does not. relational operators; safebool operator; pimpl::null (and pule-eee-ase do not tell me that *I* do not need that); support for in-place and replace boost::serialization; support for value and pointer semantics (and others if needed policies); support for the Bridge pattern; support for Non-Virtual Interface Idiom; the proper deployment of the incomplete-type management technique (for the destructors to always work correctly). I do agree that that's "not too much" as you put it. After all it's just one tiny header file. However, you seem to like implementing (or rather ignoring) that functionality for every pimpl you write. I seem to favor finding a solution for a particular problem (even if that problem is as small as a pimple ;-) ) and then *re-use* that solution. As I wrote in the original article which you obviously did not bother reading -- "Writing a conventional Pimpl-based class is not hard. However, repeating the same scaffolding over and over again is tedious and error-prone. Why do that if we do not have to?" Have a nice day. Vladimir Batov.
On 5/26/11 3:14 PM, Vladimir Batov wrote:
Well, these reviews are clearly "fun". Is it that I am getting old and touchy or these new kids on the block are getting vicious or they are learning C++ before they learn "please", "thank you" and "with all due respect"?
(off-list reply) No, it's just that Artyom can be particularly obnoxious sometimes, which is quite frustrating. :-/ Sorry about that. Cheers, Greg
On 5/26/11 5:39 PM, Gregory Crosswhite wrote:
On 5/26/11 3:14 PM, Vladimir Batov wrote:
Well, these reviews are clearly "fun". Is it that I am getting old and touchy or these new kids on the block are getting vicious or they are learning C++ before they learn "please", "thank you" and "with all due respect"?
(off-list reply)
No, it's just that Artyom can be particularly obnoxious sometimes, which is quite frustrating. :-/ Sorry about that.
Cheers, Greg
Oops... wow, I really need to learn how to use my e-mail client one of these days... Well, even though that was meant to be a private expression of sympathy rather than a public beratement of Artyom, it nonetheless is true. Artyom makes some good points in his review, but overall his comments on this library feel a lot more like an attack than a criticism, so I sympathize with Vladimir's frustration on this matter. Cheers, Greg
From: Vladimir Batov <vb.mail.247@gmail.com>
Artyom Beilis <artyomtnk <at> yahoo.com> writes: ...
Well, these reviews are clearly "fun". Is it that I am getting old and touchy or these new kids on the block are getting vicious or they are learning C++ before they learn "please", "thank you" and "with all due respect"?
Artyom, if you want to have a civilized conversation/discussion or at the very least to maintain any *pretense* of a civilized conversation/discussion, then you probably might like to have the other party *willing* to talk to you. If so, then *please* try to refrain from statements like "without a knowledge of the problem domain", "you need", "you want" and similar kind. I suspect quite a few people might find such style as rude, objectionable and arrogant.
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. However, it does not change the criticism to the library that I think can't be accepted as it does not do the stuff required from Pimpl library. I'm talking from point of view of somebody who actually uses and pushes pimpl around and I think the library had took wrong design approach. It is my opinion and I stand behind it.
... I snipped the actual manually-crafted and Pimpl-based code.
The difference is that the Pimpl-based version provides the following functionality which your "flexible" alternative does not.
Ok, I'll answer each point separately, but in general, Pimpl based should not expose implementation details. Pimpl class and normal class should not look different from user point of view. It is my opinion. It is correct design approach - decoupling an implementation from the interface. Exposing implementation details in Pimpl is bad design (IMHO)
relational operators;
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 2. The real relational operator should use implementation class and it does not provided automatically.
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.
support for in-place and replace boost::serialization;
Ok.. I need to take a look on it but in any case it requires some action from user... Isn't it?
support for value and pointer semantics (and others if needed policies);
More policies required as I mentioned above.
support for the Bridge pattern;
I don't see how it isn't supported with proposed clone_ptr.
support for Non-Virtual Interface Idiom;
No problem there for any pimpl implementation I'm aware of.
the proper deployment of the incomplete-type management technique (for 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.
I do agree that that's "not too much" as you put it. After all it's just one tiny header file. However, you seem to like implementing (or rather ignoring) that functionality for every pimpl you write. I seem to favor finding a solution for a particular problem (even if that problem is as small as a pimple ;-) ) and then *re-use* that solution.
The problem that additional parts should not be there for proper pimpl-class design as they are implementation details.
As I wrote in the original article which you obviously did not bother reading -- "Writing a conventional Pimpl-based class is not hard. However, repeating the same scaffolding over and over again is tedious and error-prone. Why do that if we do not have to?"
First of all I did read the article and I do want pimpl in Boost but not in the way this library suggests. 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) and exposes too much. - No support of non-copyable implementations (with value_semantics) - No support of custom implementation classes. Consider clss unicode_string { private: smart_ptr<icu::UnicodeString> pimpl_; }; - Better support of place holders (empty implementation object for future use) is required. - No support of cloning underlying object (instead of copying) - Thinks that are implementation details exposed to the interface, (noted above) Best Regards, and I apolagize once again if my review was rude. Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
"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.
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
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. there. So, anyone doing things differently is an illiterate moron "without knowledge of the problem domain". 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?). ;-)
… 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.
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.
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”. 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.
support for in-place and replace boost::serialization; Ok.. I need to take a look on it but in any case it requires some action from user... Isn't it?
Well, anything “requires some action from user... Isn't it?”
support for value and pointer semantics (and others if needed policies); More policies required as I mentioned above.
“My” Pimpl provides two most deployed policies. I had no substantial feedback about other policies needed. Is it a good reason enough to say that the Pimpl as a whole should be rejected? Maybe “accepted with new policies written as the need arises?” ;-)
support for the Bridge pattern; I don't see how it isn't supported with proposed clone_ptr.
support for Non-Virtual Interface Idiom; No problem there for any pimpl implementation I'm aware of.
Have you actually tried deploying the above-mentioned patterns with the example you provided along side the Pimpl-based example? If you are using some other smart pointers instead, then I won’t be surprised they provide functionality *similar* to Pimpl to support deployment of those patterns. It does not make “my” Pimpl bad. It only means that I submitted something that you have an alternative implementation of.
the proper deployment of the incomplete-type management technique (for 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.
I do agree that that's "not too much" as you put it. After all it's just one tiny header file. However, you seem to like implementing (or rather ignoring) that functionality for every pimpl you write. I seem to favor finding a solution for a particular problem (even if that problem is as small as a pimple ;-) ) and then *re-use* that solution.
The problem that additional parts should not be there for proper pimpl-class design as they are implementation details.
Our interpretations of “proper pimpl” obviously differ. Again, if you insist on denying some features to the user, use private inheritance; do not deny that functionality to others.
… First of all I did read the article and I do want pimpl in Boost but not in the way this library suggests.
I honestly want to believe you. It’s just that I felt you insistently misrepresented “my dear” Pimpl as solely shared_ptr-based Pimpl (which is not good for your use-cases) and demanded copy_ptr-based Pimpl mysteriously failing to notice value-semantics Pimpl functionality staring at you.
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. 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.
- 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.
- 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.
- Better support of place holders (empty implementation object for future use) is required.
That “design” is clearly misguided. Borrowing from one of my Internet friends it’s “wrong approach and design (IMHO)”. 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”.
- No support of cloning underlying object (instead of copying)
I am not sure if you noticed by now but “my dear” Pimpl is policy-based. If there is enough demand for that cloning, we’ll provide pimpl { typedef class pimpl_base<cloning_policy> cloning_semantics; }
- Thinks that are implementation details exposed to the interface,
Wrong again. Private inheritance shuts out *all* pimpl API from the user. So, the implementer is free to pick and choose. So, “my dear” Pimpl gives people a choice and that’s what makes it a *library*. V.
----- 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/
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.
From: Vladimir Batov <vb.mail.247@gmail.com>
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");
[snip]
... And thanks, man, for not calling me a moron "EVEN thou" you stand behind your opinion. :-)
Please, don't put words in my mouth that I have never said. Yes, Boost.Pimpl has holes in it - big holes whether you like it or not. So let's stop this "discussion" about what and who had or hadn't said. It does not bring anything. It does not forward us to a final goal to have a good pimpl library in boost.
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.
I'm aware of this. But don't you want to receive a preliminary reviews to be ready to hear what would happen on actual review? The review I had sent wasn't created to "remove" the pimpl from the review queue or to tell we don't need it, it was written to bring up real problems or issues or "personal preferences" or what ever you call. I fully understand that you like your library and your design but I suggest you to listen to others opinion. It may matter (or not). Finally, the non-formal-review at this stage only can **help** you to pass the real formal review as you would know what to expect. That's it.
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. :-)
I glad you do not review KDE's and Qt's code :-) As I can say - this is your opinion and your way of seeing things, it may differ from others. See: http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Using_... This is common practice.
... Library does not even provide an option to have a pimpl as member that is why I think it is inflexible.
[snip]
The library addresses a certain well-defined concept/issue. It is about constructing a Pimpl class easily, safely and painlessly.
It is ok, but all I'm trying to say (may be in 5th time) that pimpl is little bit more then that. It is ok that Boost would have the "pimpl" template you suggest but it should not be "Pimpl" library but a part of pimpl library. Pimpl - is much wider topic (IMHO).
- 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?
I had shown you 2-3 examples of such cases where it is not suitable but I'll repeat: a) Wrapping different implementation (not mine) with different interface class ustring { private: smart_ptr<icu::UnicodeString> pimpl_; }; b) Improving compilation speed by hiding a real object behind a lighweight wrapper. // // Real implementation uses havy classes in headers like Asio. // used in many locations, // class connection { public: ... private: boost::asio::ip::tcp::socket socket_; ... }; // // Lighweight wrapper for basic functionality that is // used in places where I don't want to include full ASIO. // to do very simple stuff where connection is the real // fully functional object. // class connector { public: ... void setip(std::string ...) connection &impl(); ... private: smart_ptr<connection> pimpl_; }; c) Not calling class "implementation" just because it only holds data memebers and not real implementations (AKA d-pointer) Only to make adding new members not to change its sizeof struct book { private: // data members - not functions. smart_ptr<data> d; }; So having pimpl<book>::implementation is misleading as it is only... discuonnected part of body - have data members. Any more arguments?
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.
Consider this case: statistics.h class statistics { public: ... void add_new_value(double v); double std() const; double mean() const; ... private: // enough to handle standard deviation and mean values int N_; double sum_; double square_sum_; // Place holder for future use // for statitics that can't be calculated with // only the values above. struct data; smart_ptr<data> d; }; statistics.cpp .. void statistics::add_new_value(double v) { N_++; sum_+=v; square_sum_ += v*v; } ... In this case you keep a smart pointer just as place holder not in use, the class is generally simple value type but you want to keep it extensible. This class does not even require memory allocation (at this point) But once when a new type of statistics is added the "d" pointer would be in use. Is this valid use case or not? ---------------------------------------------- Dear Vladimir, I'm just trying you to show many other use cases that I had met, used, deployed in my own and not-connected-to-me-at-all software. You provided a nice pimpl framework but *in my opinoin* it does not cover enough use cases. If a real formal review was running these days, you would get my negative vote. Best regards and good luck with your library. Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/
Artyom Beilis <artyomtnk <at> yahoo.com> writes: ... I glad you do not review KDE's and Qt's code
I am glad I do not have to. I've seen it. It's not exactly pretty. It's somewhat understandable as their code base has evolved over quite considerable period. Say, their signal/slot idea is so much better implemented using boost::signal rather than using some obscure macros, qt-precompiler, run-time detection of incorrect signatures. It does not make the solutions they had to resort to common practice though.
See:
http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Using_... Pointer
This is common practice.
From that URL: The d-pointer pattern has been described many times in computer science history under various names, e.g. as pimpl, as handle/body or as cheshire cat.
So, as you can see Pimpl *is* a d-pointer. It beats me why it's so important to support another d-pointer inside Pimpl (which by author's description is a d-pointer).
The library addresses a certain well-defined concept/issue. It is about constructing a Pimpl class easily, safely and painlessly.
It is ok, but all I'm trying to say (may be in 5th time) that pimpl is little bit more then that.
It is ok that Boost would have the "pimpl" template you suggest but it should not be "Pimpl" library but a part of pimpl library.
Pimpl - is much wider topic (IMHO).
I disagree that Pimpl is "much wider topic". The idiom is well-described and well-defined in various sources. I think in the original article I mentioned sources which I based my understanding of Pimpl on. I believe I was faithful to the last letter to those sources and to the Pimpl idiom definition. Could you please list the sources where you get your extended interpretation of the Pimpl idiom from? Pimpl is essentially another smart pointer and all of them (shared_ptr, unique_ptr, etc.) have their well-defined qualities, properties and applications. If we start arbitrarily "extending" those definitions and their applications beyond what those pointers were designed for, we'll end up with a mess.
... I had shown you 2-3 examples of such cases where it is not suitable but
I'll repeat:
a) Wrapping different implementation (not mine) with different interface
class ustring { private: smart_ptr<icu::UnicodeString> pimpl_; };
With my Pimpl it's done as follows: // Interface struct ustring : pimpl<ustring>::whatever { pure interface; }; // Implementation template<> pimpl<ustring>::implementation { icu::UnicodeString data; } or template<> pimpl<ustring>::implementation : public icu::UnicodeString { }
b) Improving compilation speed by hiding a real object behind a lighweight wrapper.
// // Real implementation uses havy classes in headers like Asio. // used in many locations, // class connection { public: ... private: boost::asio::ip::tcp::socket socket_; ... };
// // Lighweight wrapper for basic functionality that is // used in places where I don't want to include full ASIO. // to do very simple stuff where connection is the real // fully functional object. // class connector { public: ... void setip(std::string ...) connection &impl(); ... private: smart_ptr<connection> pimpl_; };
With my Pimpl it's done as follows: // Interface struct connector : pimpl<connector>::whatever { pure interface; }; // Implementation template<> pimpl<connector>::implementation { connection data; } or template<> pimpl<connector>::implementation : public connection { }
c) Not calling class "implementation" just because it only holds data memebers and not real implementations (AKA d-pointer)
Only to make adding new members not to change its sizeof
struct book { private: // data members - not functions. smart_ptr<data> d; };
So having pimpl<book>::implementation is misleading as it is only... discuonnected part of body - have data members.
Applying Pimpl idiom to the book above is, uhm... err, ...makes no sense to me. I'd suggest // Interface struct book : pimpl<book>::whatever { interface; } // Implementation template<> pimpl<book>::implementation : public data {}
Any more arguments?
No, I am just tired. Let's just agree to disagree.
Consider this case:
statistics.h
class statistics { public: ... void add_new_value(double v); double std() const; double mean() const; ... private: // enough to handle standard deviation and mean values int N_; double sum_; double square_sum_;
// Place holder for future use // for statitics that can't be calculated with // only the values above. struct data; smart_ptr<data> d; };
statistics.cpp .. void statistics::add_new_value(double v) { N_++; sum_+=v; square_sum_ += v*v; } ...
In this case you keep a smart pointer just as place holder not in use, the class is generally simple value type but you want to keep it extensible.
This class does not even require memory allocation (at this point) But once when a new type of statistics is added the "d" pointer would be in use.
Is this valid use case or not?
Yes, it is a valid use-case... poorly implemented. I'd apply normal Pimpl idiom, i.e. I'd separate interface from the data/implementation; I'd hide the implementation and that includes int N_; double sum_; double square_sum_; like template<> pimpl<statistics>::implementation { int N_; double sum_; double square_sum_; }; Then I'd extend pimpl<statistics>::implementation when my requirements change. In fact, I already do exactly that. If I want to avoid memory allocation, then, I'd use a policy as Gottlob Frege suggested... but I'd keep design clean and simple.
I'm just trying you to show many other use cases that I had met, used, deployed in my own and not-connected-to-me-at-all software.
You provided a nice pimpl framework but *in my opinoin* it does not cover enough use cases. If a real formal review was running these days, you would get my negative vote.
Yes, I see and understand your position and your needs (they are not really that different from mine -- I face similar issues). We happen to approach things differently. And that's OK. It's equally OK if you vote yes, no, whatever. In all honesty I do not care who and how votes... and I won't write your name into the list of my enemies. If fact, I kinda like you (even though you've been getting on my nerves lately :-) ). Equally I do not really care if Pimpl is accepted into Boost or not. Quite some time back I thought I had something simple and useful for others. So, I was willing to share it with others. If people vote it out, then they do not need it. And that's fine by me. Now I am just tired and want to get it over with one way or the other. And, yes, I admit I've been quite nasty to you lately. My honest and humble apologies. My only excuse could be that "you started it"... but I am a bit old to use that line of reasoning. So, I have no excuses. I am sorry. I wish you all the very best and I wish your pimples soundly beat mine. ;-) V.
Vladimir Batov wrote:
Artyom Beilis <artyomtnk <at> yahoo.com> writes:
a) Wrapping different implementation (not mine) with different interface [snip]
This example, showing the before and after, is just what's needed in the documentation.
b) Improving compilation speed by hiding a real object behind a lighweight wrapper. [snip]
This example, showing the before and after, is just what's needed in the documentation.
c) Not calling class "implementation" just because it only holds data memebers and not real implementations (AKA d- pointer)
Only to make adding new members not to change its sizeof
struct book { private: // data members - not functions. smart_ptr<data> d; };
So having pimpl<book>::implementation is misleading as it is only... discuonnected part of body - have data members.
Applying Pimpl idiom to the book above is, uhm... err, ...makes no sense to me.
It isn't without sense entirely. Avoiding calls to operator new until, and unless, needed, is beneficial.
I'd suggest
// Interface struct book : pimpl<book>::whatever { interface; } // Implementation template<> pimpl<book>::implementation : public data {}
Nah, it's just struct book : public pimpl<book>::whatever { private: // data members - not functions. }; Then, if and when the extra is needed, play with pimpl<book>::implementation.
Consider this case:
statistics.h
class statistics { public: ... void add_new_value(double v); double std() const; double mean() const; ... private: // enough to handle standard deviation and mean values int N_; double sum_; double square_sum_;
// Place holder for future use // for statitics that can't be calculated with // only the values above. struct data; smart_ptr<data> d; };
statistics.cpp .. void statistics::add_new_value(double v) { N_++; sum_+=v; square_sum_ += v*v; } ...
In this case you keep a smart pointer just as place holder not in use, the class is generally simple value type but you want to keep it extensible.
This class does not even require memory allocation (at this point) But once when a new type of statistics is added the "d" pointer would be in use.
Is this valid use case or not?
Yes, it is a valid use-case... poorly implemented.
IIUC, statistics can be extended with new, non-virtual member functions in a future release. So long as the size is unaffected, then old and new clients can use the new version without change because the object's size is not affected. The new version, however, must allocate and initialize the extra state and the new member functions would use that new state for their computations.
I'd apply normal Pimpl idiom, i.e. I'd separate interface from the data/implementation; I'd hide the implementation and that includes int N_; double sum_; double square_sum_;
like
template<> pimpl<statistics>::implementation { int N_; double sum_; double square_sum_; };
Then I'd extend pimpl<statistics>::implementation when my requirements change.
Sure, that works. The down side is that the implementation object *must* be allocated on the free store from the start. With Artyom's version, that only occurs *if* extra state becomes necessary.
In fact, I already do exactly that. If I want to avoid memory allocation, then, I'd use a policy as Gottlob Frege suggested... but I'd keep design clean and simple.
Using SBO, things work very nicely but for one problem. Artyom's version permits inline member functions that access the data members not in the implementation object. Is that necessary? It depends. Nevertheless, Artyom's approach can be supported by leaving the implementation null until, and unless, it is needed. IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
Vladimir Batov wrote:
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.
This is getting to be ridiculous. Artyom came on too strong and apologized. You, Vladimir, are the one that keeps harping on things. You could have simply apologized for putting words in Artyom's editor, but that's not enough. You aren't content to stop there as the "patronizing" and "scolding" paragraph shows.
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?
This criticism is well founded, but I think Artyom has corrected his tone since you pointed out problems last week. It doesn't seem that you have changed likewise.
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.
There is room to teach, if you are knowledgeable and the tone is respectful and civil.
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.
I'm guilty of assuming, and strongly asserting, that I'm right until proven otherwise. In a similar vein, I think Artyom came off stronger than he intended and that you took his words as even stronger. Let's assume that English as a second language, the written word lacking visual and audio cues, and lack of personal knowledge are all factors in the communications problems for all concerned and just focus on the technical discussion. Please!
Secondly, it's not a review. There is no Review Manager, there is no time frame, there is no outcome/resolution.
Your library is in the review queue. That means that you assert that it is review ready. That also means that folks can write early reviews. Therefore, Artyom's review was legitimate.
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.
This is an unwarranted personal affront directed at me now. I certainly wanted more opportunity for the community to test Code Collaborator. Your library was in the review queue. I had looked at it while at BoostCon during the Library in a Week session and had comments I wanted to share. Code Collaborator seemed like a good way to do it. I also saw this as a chance to get others to look at your library in detail. As the author of a proposed Boost library, I assumed you would welcome feedback. I veiled nothing. I asked whether you would like to put your tool in Code Collaborator to permit me to provide some feedback in a form I found useful while at BoostCon. I suggested that others might likewise be interested in a detailed look at the library so I suggested that you post a link to the developer's list. There was *nothing* in that which comes close to being a trap. How early feedback, of the sort you otherwise would only get during a review, is of "seemingly no particular gain for" you is beyond me.
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.
That doesn't account for the cases in which there is nothing extra needed which means that book is a value type and has known size with nothing extra on the free store. Later, if book must be extended in a future version, the extra state can be put in added_. Until then, added_ is null. As I've noted, however, this can be done by inheriting from pimpl<book> and only defining and instantiating pimpl<book>::implementation when that becomes necessary. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
Artyom Beilis wrote:
Vladimir Batov <vb.mail.247@gmail.com> wrote:
"Artyom Beilis" wrote
… 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.
That's true. A using declaration can address that, but it must refer to pimpl<derived>::operator ==(), which is hardly helpful to the derived class' user to understand that it provides an equality operator. The solution works but isn't satisfactory.
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.
We need to get more precise in these discussions. I think Artyom is referring to the same issue that I have raised. Referring to your article, we're saying, if I'm not putting words in Artyom's mouth, that Book's interface should be unchanged regardless of whether pimpl is used. The interface exposed for Book to use in its implementation is separate. IIUC, you think that there are legitimate use cases for extending Book's interface. If that survives review, then I think policies are needed to capture the distinct needs more directly rather than requiring private inheritance plus using declarations.
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”.
Using words like "friend" doesn't mean you aren't being caustic. Calling his idea "rubbish" is uncivil. Describing your answer as "the correct" one implies his is wrong. You can make your point with just, "The answer really depends upon usage patterns."
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.
This is good information. Indeed, I know you specifically support value semantics despite not really needing it yourself. The question seems to be whether that support is what those using value semantics expect.
I’ll give you a simplified example
Good
(so do not jump in with “you all are doing it all wrong!”).
Let's assume everyone will be civil now. This is out of place with that mindset.
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.
This is an interesting use case, but I'm not certain but what you're expecting the Pimpl Idiom to solve two problems at once. IOW, what I read in your example is the Bridge Pattern, not Pimpl. Both use a pointer to an implementation class, but the former expressly includes the idea of sharing the implementation class. Pimpl, by contrast, is about moving the private state of a class into the implementation file to hide it from the class definition and its clients. Unless my memory is worse than I have come to expect, I don't recall Sutter ever suggesting that using Pimpl should alter the interface of the class employing it.
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…
If I'm right that you really are using your pimpl library to implement the Bridge Pattern, then calling it by that name would probably eliminate the overreaching concerns that have been voiced. (It would also imply a separate library.)
although for unknown reasons you are adamant in not noticing that “my” Pimpl offers the functionality you are after.
Might the problem be that it is in a form that doesn't look like the solution he's expecting? If so, then it's a matter of documentation at least.
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
You didn't really stop short, did you? You wrote want you really wanted to say and then pretended that you didn't. This isn't helping to keep things civil.
For value-semantics Pimpls you are probably correct for the majority of cases.
Given that admission, it is reasonable to consider what the value semantics API should add to the derivate.
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.
If you care: s/nick/neck/ If you agree that you're actually using the Bridge Pattern, then perhaps the rules are closer to what Artyom has expressed.
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);
Maybe not, if you're really just using the Bridge Pattern.
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.
Are you saying that you very much think that Book's clients should have access to Book::null()? I don't deny that there are classes that provide a named constructor or a default constructor to create an "empty" instance that must be made usable via some secondary construction or assignment operation, but why must that be part of the Pimpl Idiom? I have never seen this associated with Pimpl before and it seems wrongheaded for the idiom. Maybe I'm just being dense.
if you want to deny it to your users, use private inheritance. Just do not deny it to the whole world.
But as I noticed before private inheritance would remove some other things that in some cases you may use.
The question is whether the Pimpl Idiom includes that functionality.
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.
There is great value in limiting what a library does if it prevents misuse. In this case, we're questioning your pointer semantics idea. It may be that you've convolved ideas that should be separate. If we're right and we convince you of that, then the library will be better for it because it will prevent others from doing the same thing. If we're wrong, then we have to be convinced which will only help you to improve your understanding of how to document those use cases. Finally, it could be simply a matter of naming. Perhaps your pointer semantics pimpl is really "bridge."
- 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.
This is unfair, if I have things straight. Your earlier response to this point was earlier in the same message. Thus, you can't presume he would have altered course mid-message.
- Better support of place holders (empty implementation object for future use) is required.
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.
Qt might have code that does what you're asking, but that doesn't mean a Boost library should encourage that style.
Placeholders are critical for keeping stable ABI.
The option to manage a data member for possible, future needs isn't necessary. You can derive from pimpl<derivate> and choose to not define or instantiate pimpl<derivate>::implementation until it is needed. The effect is the same, assuming we figure out the API and semantics. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
participants (5)
-
Artyom Beilis -
Gregory Crosswhite -
Ivan Le Lann -
Stewart, Robert -
Vladimir Batov