Boost logo

Boost :

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


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