Re: [boost] [pimpl] Mini Review

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

Artyom Beilis wrote:
> Vladimir Batov <vb.mail.247_at_[hidden]> 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_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.