Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-05-31 11:42:20

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_at_[hidden]
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.