Re: [boost] [pre-review] Pimpl submission in the review queue

Subject: Re: [boost] [pre-review] Pimpl submission in the review queue
From: Jeff Garland (azswdude_at_[hidden])
Date: 2011-05-24 14:49:01

Mostafa wrote:
>> If you want to use the Code Collaborator site to examine the library, 
>> you must create an account.  The good news is that there are other 
>> libraries there that you can review, too.  If you wish to avoid 
>> creating an account, then you can follow the link on the review 
>> schedule to the code in the sandbox.
> 
> Some may just want to follow a review without necessary participating in 
> one.  Requiring a user account discourages such activity, and in my 
> opinion, creates an artificial barrier to further engagement between 
> newcomers, or even existing Boost watchers, and Boost, even if it is a 
> one-sided engagement.
> 
> My thoughts,
> 
> Mostafa

This is all getting a bit ahead of my post-boostcon report on the library in a 
week review activities, but to answer this particular issue I'd say 1) you 
need a login to participate in the mailing list, 2) it's essential for spam 
prevention, and 3) data can be periodically put to the list to mitigate this 
issue.  Below is a report (minor formatting tweaks) of the pimpl review 
generated from code collaborator.

I think in all likelihood using one of the modern review tools is going to 
create a better environment overall.

Jeff

*************
Overview
ID:	5	Status:	Inspection
Title:	Pimpl idiom generalization.	Creator:	Vladimir Batov
Created On:	2011-05-23 15:13	Finished On:	2011-05-24 11:55
Total Person Time:	01:27:06	Wall-Clock Time:	20:34:38
Total Reviewer Time:	00:46:00	LOC All Versions (Uploaded/Changed):	2997/0
LOC Final Versions (Uploaded/Changed):	2997/0	Document Pages (All):	0
Document Pages (Final):	0	Number of Defects:	16
Number of Comments:	8	

*************
Overview:	

Support for value and pointer semantics. Support for boost::serialization. 
Support for separate -- interface and implementation -- class hierarchies (the 
Bridge pattern in GoF). Support for internal (implementation-only) run-time 
polymorphism (Non-Virtual Interface Idiom). Support for delegating 
constructors. Tutorial. Doxygen-generated docs. Linux and VS2008.

*************
Participants

Name	Role	Person-Hours
Vladimir Batov	Author	00:13:17
Robert Stewart	Reviewer	00:46:00
Sean Chittenden	Observer	00:06:13

*************
Defect Log
ID	Creator	File	Location	Type	Severity	Text

D22	Robert Stewart	pimpl.hpp	Line 25	Interface	Major	Names with two 
consecutive underscores are reserved for the implementation

D23	Robert Stewart	pimpl.hpp	Line 25	Style	Minor	This macro name should start 
with "BOOST_PIMPL_"

D24	Robert Stewart	pimpl.hpp	Line 54	Interface	Minor	pimpl<T>::xxxx_semantics 
is interesting, but xxxx_pimpl<T> is simpler and more direct.

D25	Robert Stewart	pimpl.hpp	Line 157	Interface	Major	Names with double 
underscores are reserved for the implementation

D26	Robert Stewart	pimpl.hpp	Line 158	Interface	Major	Names with double 
underscores are reserved for the implementation

D27	Robert Stewart	pimpl.hpp	Line 159	Interface	Major	Names with double 
underscores are reserved for the implementation

D28	Robert Stewart	pimpl.hpp	Line 160	Interface	Major	Names with double 
underscores are reserved for the implementation

D29	Robert Stewart	pimpl.hpp	Line 244	Interface	Major	Names with double 
underscores are reserved for the implementation

D30	Robert Stewart	pimpl.hpp	Line 49	Interface	Minor	What about Pimpl support 
for classes with a base class? Your design requires multiple inheritance. You 
could, instead, do the following:

class Book : public value_pimpl<Book, Base>
{
};

template <class T, class Base = null_type>
class value_pimpl : public Base
{
};

template <class T>
class value_pimpl<T,null_type>
{
};

D31	Robert Stewart	pimpl.hpp	Line 291	Interface	Minor	This exposes too much of 
the implementation to the user. A construct() member function set, like the 
set of constructors your macro now generates, with perfect forwarding (and 
variadic support, when available), would be better. construct() can call the 
new operator and save the pointer, thereby hiding those details from the user.

D32	Robert Stewart	pimpl.hpp	Line 57	Interface	Minor	This exposes the 
pseudo-smart pointer semantics. It shouldn't be public and it should probably 
be named "nil" rather than "null." The idea is that nil() returns an object 
with no implementation rather than a null pointer.

D33	Robert Stewart	pimpl-use-cases.html	Line 346	Documentation	Minor	"It is 
not but not a 100% kosher" is nonsensical. Presumably, you mean something 
like, "This design has some problems."

D35	Robert Stewart	pimpl-use-cases.html	Line 357	Documentation	Minor	s/nut 
shell/nutshell/

D36	Robert Stewart	pimpl-use-cases.html	Line 364	Documentation	Minor 
s/More,/What's more,/

D37	Robert Stewart	pimpl-use-cases.html	Line 385	Documentation	Minor	s/for 
vast majority/for the vast majority/

D38	Robert Stewart	pimpl-use-cases.html	Line 386	Documentation	Minor	You can 
avoid the subsequent "he/she" awkwardness by rephrasing this sentence:

"However, putting both <i>book1</i> and <i>book2</i> in a <i>std::set</i> 
results in two elements rather than one, despite the fact that they are 
supposed to represent the same <i>Book</i>."

Overall Review Conversation

Sean Chittenden	I've been using Boost.Pimpl for almost a year now with great 
success (using it in conjunction with MSM, ASIO and Spirit). This has been an 
incredibly useful library for minimizing the interfaces exposed across 
components. There are three things that I'd like to see but are largely 
trivial: 1) a set of clean-room examples of the two implementations (i.e. 
split out the unit tests from a set of examples for pointer and value 
semantics), and an example of how to use Pimpl once it has been moved in to 
the boost namespace (I took a crack at this and was never successful at moving 
Pimpl in to the boost namespace).