Re: [boost] Review Request: impl_ptr (pimpl)

Subject: Re: [boost] Review Request: impl_ptr (pimpl)
From: Gary Furnish (gfurnish_at_[hidden])
Date: 2017-08-22 15:06:34

While I like the idea in general (bikeshedding aside), it should not
be possible to end up with nulls when treating something as a value
type.  All of that should be hidden away with some sort of enable if
machinery.  Also your support for allocators doesn't work properly
because if you use something like an allocator that returns offset
pointers you assume that all allocators return normal pointers.

On Tue, Aug 22, 2017 at 5:49 AM, Vladimir Batov via Boost
<boost_at_[hidden]> wrote:
> Andrzej,
>
> I'll just answer top-post as we are not exactly arguing any particular
> implementation/documentation points. We just happen to see things from
> somewhat different angles. I understand that you've had bad experiences with
> shared_ptrs and such and nulls. Fair enough. So, if you do decide to use
> impl_ptr library, you'll avoid impl_ptr::shared policy and will use others.
> From what I understand those "unique", "copied", "onstack" policies should
> fit your needs and address your null-related concerns. Your concerns of
> objects being uninitialized (null) are unfounded. We can discuss it later in
> depth when the time comes.
>
> As for the docs, then they certainly can and should be improved... is now
> the time to spend time improving them?.. I personally suspect not because
> impl_ptr library is in the "endorsement request" stage... it is not even a
> review stage but some strange pre-review stage... and so far I cannot see it
> making any further. So, we'll address issues raised when it is appropriate
> (given that those issues so far have not been serious architectural/design
> issues but documentation and potential misunderstanding).
>
> Best,
> V.
>
>
>
> On 2017-08-22 18:09, Andrzej Krzemienski via Boost wrote:
>>
>> 2017-08-22 2:34 GMT+02:00 Vladimir Batov via Boost
>> <boost_at_[hidden]>:
>>
>>> On 08/21/2017 10:38 PM, Andrzej Krzemienski via Boost wrote:
>>>
>>>> 2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost
>>>> <boost_at_[hidden]
>>>>
>>>>> After much procrastination I would like to request a formal review for
>>>>> the pimpl library...
>>>>>
>>>> I find it disturbing that the first example describes using shared_ptr-s
>>>> and defensive null checks. Boost has always been promoting the STL-like
>>>> value-semantic style of programming, and this usage of shared_ptr's
>>>> looks
>>>> counter to the trend. When I am defining type Book -- unless I have to
>>>> hide
>>>> implementation, I will just put the members directly in order to achieve
>>>> value semantics, in particular, I want to guarantee the following
>>>> property:
>>>> Book b1 {"title", "author"}; Book b2 = b1; Book b3 = b1; modify(b1);
>>>> assert
>>>> (b2 == b3); assert (b2 != b1); The shared_ptr does no seem to guarantee
>>>> the
>>>> last assertion. Also, there are these additional members testing if
>>>> pimpl
>>>> is null, which appear suspicious to me. How can it be null? Do I have to
>>>> implement the default constructor now? Does this also mean than in my
>>>> types
>>>> that I want to pimpl-ize I cannot use operator bool for my own
>>>> business-logic-related purpose, because it is already taken by the
>>>> implementation of boost::pimpl? There may be good use cases for
>>>> flyweight-like implementation, but it should not necessarily be the
>>>> first
>>>> example we see in the docs.
>>>>
>>>
>>> Andrzej,
>>>
>>> To be honest I am somewhat surprised... in part by the fact that you find
>>> the first example no less than "disturbing". I do hear your view and I
>>> find
>>> it is a perfectly valid view. Still, I am sure you've been around the
>>> bush
>>> for long enough to know that there are as many views/opinions/preferences
>>> as there are people. Then different projects have different requirements.
>>> So, there should not be anything "disturbing" if you see someone doing
>>> something his way rather than yours to satisfy the requirements of their
>>> project rather than yours. Don't you agree?
>>>
>>> Then you go on to explain that the shared-properties pimpl does not suit
>>> your requirements and to describe your potential confusion (and even
>>> frustration) with such pimpls... Where all that is coming from? No one is
>>> forcing you to use the shared-properties pimpl, right?
>>> Exclusive-ownership
>>> pimpls are equal-rights' citizens in the implementation. They are equally
>>> presented in the documentation. What is there to be "disturbed" by?
>>> Indeed,
>>> in the documentation I do mention/describe the shared-properties pimpl
>>> first... because it is easier to introduce the Pimpl concept using
>>> std::shared_ptr. Different people have different skill/knowledge levels,
>>> different expectations, different priorities, different reading styles.
>>> There is no way to satisfy them all. Please do not be "disturbed" if you
>>> do
>>> not see things done exactly you'd do that.
>>>
>>
>> Part of my complaint is indeed coming from my personal judgement of
>> different implementation strategies. The other part is me giving you a
>> feedback from the "user experience" of a potential user of your library. I
>> am an impatient programmer who is facing alternative: implement pimpl
>> manually or use a third-party library, there might be like 100 libraries
>> for this purpose with different quality, and I have very little time to
>> asses if it is better to use one of them, or to write my own. Having
>> encountered a library I need to be able to answer a couple of questions
>> quickly, like, is this library worth investing time in even reading the
>> full documentation. So, one of the first questions I would like being
>> answered is. Is the author of this library aware of the present
>> programming
>> styles, like value semantics, zombie-object avoidance, or is (s)he
>> confined
>> to these older OO-like techniques, where everything is pointers and
>> garbage-collected (or shared_ptr-collected) memory with null pointers
>> causing bugs?
>>
>> I cannot figure this out from the docs of Boost.Pimpl quickly. Ultimately,
>> I can figue it out from studying the entire docs, or the implementations,
>> or private conversation with you. But: I cannot figure it out *quickly*
>> from the *initial pages* of the docs. First page: no information -- I know
>> what pimpl is already. Second page: "the basics" -- I do not want the
>> basics. I want answers, preferably in form of a short example. There is a
>> word "unique-ownership" highlighted, so this is something, but still some
>> questions unanswered: will I have null pointers/null states all around?
>>
>> Third page: I get an example of something I would not like to get. I give
>> you that it your order of presenting policies is no worse than my expected
>> order of policies, but I can see these two functions in the interface for
>> testing for the null state. Note that separating interface from
>> implementation has nothing to do with offering an additional null state.
>> The two things are orthogonal; it is just that it is easy to provide a
>> null
>> state when you are implementing a handle anyway -- but they are different
>> things, and they distract attention. It is not clear from the first
>> glimpse
>> at the example whether I am forced to have the null state or whether I can
>> opt-in to have it.
>>
>> By that time I will have exhausted the time I was willing to devote to
>> studying the library usefulness, and would move to the next library or to
>> implementing my own pimpl.
>>
>> Note that my remarks are to the structure of the initial pages of the
>> docs:
>> not to your design choices.
>>
>>
>>> > Also, there
>>> > are these additional members testing if pimpl is null, which appear
>>> > suspicious to me. How can it be null?
>>>
>>> std::optional has relational operators and can be "null", invalid.
>>> std::shared_ptr() has relational operators and can be "null", invalid.
>>> unique_ptr has relational operators and can be "null", invalid. The class
>>> I
>>> proposed -- impl_ptr -- has relational operators and can be "null",
>>> invalid. What is so suspicious about it? The name itself -- impl_ptr --
>>> highlights the fact that it is a smart pointer, a handle/body construct.
>>> So, the handle can be associated with a body... or the handle might not
>>> have a body, i.e. "invalid", "null".
>>
>>
>>
>> Of optional, I expect a null state. This is what optional is for and  this
>> is reflected in its type. Of shared_ptr I do not necessarily expect a null
>> state, and I treat it as necessary evil, and long for types with shared
>> semantics but without null state which is causing many bugs in my programs
>> I work with. Of "pimpl" library I expect separation of interface and
>> implementation: not a null state. If you provide it, I would expect a
>> clear
>> indication that it is an opt-in feature.
>>
>>
>>>
>>> > Do I have to implement the default constructor now?
>>>
>>> No, you do not. Your frustration seems palpable. I fail to understand how
>>> possibly I could cause that.
>>>
>>
>> It is not you causing it. Maybe I am just a nerd. I have bad experience
>> with having to spend lot of time in my company fixing bugs after people
>> who
>> overused OO and shared_ptrs. I see now the docs of Boost.Pimpl
>> unnecessarily promoting these. Doing flyweighs requires taking care of
>> oher
>> things like using shared_ptr<const T> rather than shared_ptr<mutable T>,
>> which the docs do not cover.
>>
>>> > Does this also mean than in my types that I want to pimpl-ize
>>> > I cannot use operator bool for my own business-logic-related purpose,
>>> > because it is already taken by the implementation of boost::pimpl?
>>>
>>> No, it does not mean anything like you describe... and it not "taken". It
>>> is provided. If you want your "own business-logic", you implement it and
>>> you use it. There is nothing stopping you from doing that, right? Unless
>>> I
>>> missed, mis-understood something.
>>
>>
>> Myabe you could reflect that in the initial examples by not providing
>> `operator bool` and `operator!`? BTW, you probably do not need `operator!`
>> anyway. The negation works out of the box when you only define explicit
>> conversion to bool.
>
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost