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

Subject: Re: [boost] [pre-review] Pimpl submission in the review queue
From: Jeff Garland (jeff_at_[hidden])
Date: 2011-05-24 15:13:04

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).