Boost logo

Boost :

Subject: Re: [boost] [pimpl] Mini Review
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2011-05-29 03:26:32


> From: Vladimir Batov <vb.mail.247_at_[hidden]> > > 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_a_d-Pointer 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/


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk