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

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.