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_interfa... 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_exclusi...), 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_i..., 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.