Boost logo

Boost :

Subject: Re: [boost] Review Request: impl_ptr (pimpl)
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2017-08-21 08:08:16


On 15/08/2017 12:51, Vladimir Batov wrote:
> After much procrastination I would like to request a formal review for
> the pimpl library... once and for all :-) (given it's been through a few
> almost-reviews and discussions)... to get some kind of a resolution one
> way or the other. :-)
[...]
> The code and the docs are at https://github.com/yet-another-user/pimpl
> <https://github.com/yet-another-user/pimpl>.

On a quick pass through the docs,
http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/behind_the_interface.html
suggests that Book::operator== be implemented using (*this)->title etc.

Given the implementation on the value-semantics page
(http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/pimpl_with_exclusive_ownership_p.html),
at first glance this *looks* like you're comparing the Book::title()
member pointers, which is obviously wrong.

Now, I know from the other comment on the former page that this is
actually an idiom for accessing the implementation's member and not the
public member, but this seems too confusing and error-prone, especially
given the common names.

I would recommend not overloading operator-> and operator* for impl
access and instead requiring that this be more explicit, such as
impl().title (or as the page also suggests impl.title, but after a
glance at the implementation I didn't see how that would work).

 From
http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/construction_and_impl_ptr_null.html,
perhaps consider implicit construction from nullptr_t as another method
to create an empty instance; that might be more natural for people.

Also, the generated reference docs seem a bit useless --
http://yet-another-user.github.io/pimpl/doc/html/reference.html shows
only two types, including _internal_impl_ptr, which is not itself
documented and just 404s.


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