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/