Mostafa wrote:

On Mon, 23 May 2011 14:42:06 -0700, Vladimir Batov <vb.mail.247@gmail.com> wrote:

...

I have a Pimpl generalization submission getting close to the top in the review queue. Rob Stewart suggested I'd post it to the new Boost Code Collaborator site for a pre-review. That's what I did with the following link to the pre-review (Review #5):

<http://demo.smartbear.com/boost/go?page=ReviewDisplay&reviewid=5>

Your participation in that pre-review is most welcome.

Is it possible to have anonymous read-only access to it?

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. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

2011/5/24 Klaim - Joël Lamotte <mjklaim@gmail.com>:

[...] This review tool seems powerful (while a bit too verbose maybe) [...]

In case you are referring to the yellow boxes of text, it's the tutorial mode, which is ON by default when you create an account I believe. Go into Prefs (top-right), Display tab, and turn off Tutorial Mode. (I'm referring to a v5 install, so v6 or later might be slightly different). Hope this helps, --DD

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

On Tue, 24 May 2011 11:53:55 -0700, Michael Caisse <boost@objectmodelingdesigns.com> wrote:

On 05/24/2011 10:59 AM, Mostafa wrote:

...

On Tue, 24 May 2011 04:16:00 -0700, Stewart, Robert <Robert.Stewart@sig.com> wrote:

...

Mostafa wrote:

...

On Mon, 23 May 2011 14:42:06 -0700, Vladimir Batov <vb.mail.247@gmail.com> wrote:

...

I have a Pimpl generalization submission getting close to the top in the review queue. Rob Stewart suggested I'd post it to the new Boost Code Collaborator site for a pre-review. That's what I did with the following link to the pre-review (Review #5):

<http://demo.smartbear.com/boost/go?page=ReviewDisplay&reviewid=5>

Your participation in that pre-review is most welcome.

Is it possible to have anonymous read-only access to it?

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

I'm going to strongly disagree here. Of late, there have been multiple reviews that have only been conducted on the dev list. While I disagree with the decision, I can both appreciate and understand it. The Review Manager role is hard enough while monitoring one list during an energetic review. However, this choice by some Review Managers has resulted in disenfranchised user-only-list members of the community. Just look at the responses on the user list when these announcement were made.

The offered solution: subscribe to the dev list for the duration of the review. The push-back to the suggestion: the dev list is a high-volume list that people don't want to wade through to get review updates from.

A tool such as Code Collaborator is the perfect solution. If you don't want the traffic of the review, don't subscribe. If you want to follow the review, then subscribe and only get updates on the review you want to follow.

There is no barrier to creating a user account. Credit card numbers are not solicited, spam is not sent. The review process is something that can benefit from tools. Please don't create FUD.

michael

Hello Michael, I think you've misread this discussion, or I haven't been very clear in expressing myself. My original, and only suggestion, was to have read-only access to Code Collaborator without the need to create an account, without making a value judgement on whether Code Collaborator was good or bad, or whether the review process should be changed or not. It may truly be that what I point out leads to FUD, but I do think it is a legitimate concern. Please see my response to Robert Stewart for more clarification: http://article.gmane.org/gmane.comp.lib.boost.devel/219704 Mostafa

On Tue, 24 May 2011 12:07:29 -0700, Stewart, Robert <Robert.Stewart@sig.com> wrote:

Mostafa wrote:

...

Rob Stewart wrote:

...

Mostafa wrote:

...

...

Is it possible to have anonymous read-only access to it?

If you want to use the Code Collaborator site to examine the library, you must create an account.

Note that creating an account does not mean your name is added to the list of reviewers. An account, along with a URL, like <http://demo.smartbear.com/boost/go?page=ReviewDisplay&reviewid=5>, gives you read-only access.

...

...

If you wish to avoid creating an account, then you can follow the link on the review schedule to the code in the sandbox.

I should have said that this was the approach to take if you wanted to avoid Code Collaborator altogether. (I don't know what's possible with Crucible yet.)

...

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.

Do you really think creating an account with nothing but an e-mail address and password will keep anyone truly interested in a review from using the tool?

Maybe not the *truly* interested, but certainly the mildly or lightly interested, at least in my case. For example, I followed Boost discussions on and off for approximately two years before I found the need to sign up to the mailing list. It maybe that I represent a minority use case, I don't know. However, I do think the easier it is for programmers at large to see what goes on in the Boost process, the better off it will be for Boost in the long run, in terms of exposure and potential contributions. And I believe requiring a user account is an impediment to such an exposure. My intention isn't to pass a yay/nay value judgement on this particular software, or any potential changes to the review process. Rather, I thought it was relevant to point out a potentially negative consequence of this tool. As an aside, there are many technical forums that don't require a user account unless the said user wants to contribute something, and, in my opinion, that is the model Boost should follow for maximum exposure. Mostafa

Artyom Beilis wrote:

...

From: Vladimir Batov <vb.mail.247@gmail.com>

Few points on the tool:

1. The interface is unfriendly, it is not clear how to write review.

You don't.

2. There is no place for general feedback, tickets are not good model.

Quite right.

3. There is no separation into general categories according to the way the review generally handled: Design, Implementation, Documentation etc.

Especially notes on design are missing.

There is some room for that, particularly by attaching comments to a line declaring a class, for example.

4. There is no way to do a discussion on specific topics.

Indeed.

5. There is no place (at least I hadn't found one) to put a vote.

Of course.

This tool would discourage reviewers and make writing reviews much harder.

Review is a document that describes the review author's opinion on the library that includes a list of issues, it is not a list of issues only.

Bug-tracking-like system is not suitable for writing reviews.

All of these are valid points, supposing a tool like Code Collaborator were intended to do what you note it doesn't. Instead, tools like this provide a means to make comments on specific lines of code or documentation, which are often rather difficult to put into prose or to track once they are. Furthermore, this particular thread regards a *pre-review* of a library, giving you and the author a chance to iron out issues before the formal review begins. Normal library reviews, with the usual questions including whether to accept the library or not, are not supposed to be submitted via such a tool. Doing so would require a highly specialized tool that I've not seen anywhere. Whether normal reviews are managed via web form, with which we experimented at BoostCon, or are done as now via posts to the mailing list(s), is unclear. Perhaps both will be supported. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

Artyom Beilis wrote:

...

From: "Stewart, Robert" <Robert.Stewart@sig.com>

...

Normal library reviews, with the usual questions including whether to accept the library or not, are not supposed to be submitted via such a tool. Doing so would require a highly specialized tool that I've not seen anywhere.

I see, if so it is clear that this is not a review tool.

It is a code review tool, not a library review tool. It has a place in the process. _____ Rob Stewart robert.stewart@sig.com Software Engineer using std::disclaimer; Dev Tools & Components Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.