[Scope] Peer review conclusion
Dear Boost community, the formal review of Boost.Scope has taken place between 26th of November and 5th of December of the previous year. I have received 5 reviews in total, including 4 here on the mailing list and 1 on Reddit. Two of the reviews recommended unconditional, and 3 conditional acceptance. The reviews came from: - Klemens Morgenstern, - Janko Dedic, - Julien Blanc, - David Sankel, - /u/jonesmz on Reddit. Also, several people have given insightful comments but did not provide a review. I would like to thank all of the people who took their time to write reviews or participate in the discussions. Your effort was very important and valuable. The outcome of the review is that Boost.Scope is ACCEPTED with several minor CONDITIONS. 1. noexcept specification on destructors of scope guards should be properly documented. Andrey has already commented that the technical hurdle that prevented it was eliminated. 2. There should be a good motivating example for scope_success. There was a lot of comments regarding scope_success and its seeming uselessness. Without a good example I see the same pattern occurring with future users. 3. The examples should better describe what they are trying to achieve. Several comments about examples suggested incorrect changes, which signals that readers failed to understand them. After a lot of consideration I decided against requesting the author to remove unique_resource and related functionality from the library. While several reviewers wanted a more "concise" library, all noted that the component is useful. One last thing I wanted to point out is that all reviewers and most commenters were adamant that compatibility with the Technical Specification on which this library was initially based should not be a goal. In fact, Andrzej Krzemienski even noted that such compatibility has prevented him from adequately evaluating the library on its own merit. In my opinion this should be a sign to all of us that people prefer better APIs over standard APIs. I recommend Andrey to re-evaluate those parts of the library's APIs that he copied from the TS and potentially pick better names for member functions. Thanks to Andrey for submitting this library to Boost. And thanks again to the reviewers and everyone who provided feedback, who ultimately made this review possible.
On 1/15/24 22:50, Дмитрий Архипов via Boost wrote:
Dear Boost community, the formal review of Boost.Scope has taken place between 26th of November and 5th of December of the previous year. I have received 5 reviews in total, including 4 here on the mailing list and 1 on Reddit. Two of the reviews recommended unconditional, and 3 conditional acceptance. The reviews came from: - Klemens Morgenstern, - Janko Dedic, - Julien Blanc, - David Sankel, - /u/jonesmz on Reddit.
Also, several people have given insightful comments but did not provide a review. I would like to thank all of the people who took their time to write reviews or participate in the discussions. Your effort was very important and valuable.
The outcome of the review is that Boost.Scope is ACCEPTED with several minor CONDITIONS.
1. noexcept specification on destructors of scope guards should be properly documented. Andrey has already commented that the technical hurdle that prevented it was eliminated.
2. There should be a good motivating example for scope_success. There was a lot of comments regarding scope_success and its seeming uselessness. Without a good example I see the same pattern occurring with future users.
3. The examples should better describe what they are trying to achieve. Several comments about examples suggested incorrect changes, which signals that readers failed to understand them.
After a lot of consideration I decided against requesting the author to remove unique_resource and related functionality from the library. While several reviewers wanted a more "concise" library, all noted that the component is useful.
As a potential option, we could split the library in two git modules, one with scope guards and another one with unique_resource. This would count as two libraries, so I'm not sure if this would be an acceptable result of the review, but if people want this, I wouldn't mind. Dmitry, please let me know if this is desired, so that I make the split. Name suggestions are also welcome.
One last thing I wanted to point out is that all reviewers and most commenters were adamant that compatibility with the Technical Specification on which this library was initially based should not be a goal. In fact, Andrzej Krzemienski even noted that such compatibility has prevented him from adequately evaluating the library on its own merit. In my opinion this should be a sign to all of us that people prefer better APIs over standard APIs. I recommend Andrey to re-evaluate those parts of the library's APIs that he copied from the TS and potentially pick better names for member functions.
Thanks to Andrey for submitting this library to Boost. And thanks again to the reviewers and everyone who provided feedback, who ultimately made this review possible.
Thank you Dmitry for managing the review and, of course, to everyone who provided their feedback, especially in the form of reviews. I will work on addressing the review comments and conditions and will post separately when I think I'm done.
вт, 16 янв. 2024 г. в 00:12, Andrey Semashev via Boost
As a potential option, we could split the library in two git modules, one with scope guards and another one with unique_resource. This would count as two libraries, so I'm not sure if this would be an acceptable result of the review, but if people want this, I wouldn't mind.
Dmitry, please let me know if this is desired, so that I make the split. Name suggestions are also welcome.
No, I did not make that a condition to acceptance. But if you feel like this is necessary, you of course are free to do that. Although, I would think, this would require a separate formal review for a new library. Just to reiterate, here's my reasoning for not making this a condition. While some people wanted unique_resource out, everyone remarked that it is useful. Some people also remarked that potentially a better API is possible. In my opinion, having a useful unique_resource in Scope does not prevent a better unique_resource in a different library later. One could argue, it would even be increase the chances of that, because real life experience can be gathered. And finally, while everyone likes lean libraries, in my experience most C++ users do not like micro libraries, because managing dependencies is not trivial.
On 1/15/24 22:50, Дмитрий Архипов via Boost wrote:
Dear Boost community, the formal review of Boost.Scope has taken place between 26th of November and 5th of December of the previous year. I have received 5 reviews in total, including 4 here on the mailing list and 1 on Reddit. Two of the reviews recommended unconditional, and 3 conditional acceptance. The reviews came from: - Klemens Morgenstern, - Janko Dedic, - Julien Blanc, - David Sankel, - /u/jonesmz on Reddit.
Also, several people have given insightful comments but did not provide a review. I would like to thank all of the people who took their time to write reviews or participate in the discussions. Your effort was very important and valuable.
The outcome of the review is that Boost.Scope is ACCEPTED with several minor CONDITIONS.
1. noexcept specification on destructors of scope guards should be properly documented. Andrey has already commented that the technical hurdle that prevented it was eliminated.
2. There should be a good motivating example for scope_success. There was a lot of comments regarding scope_success and its seeming uselessness. Without a good example I see the same pattern occurring with future users.
3. The examples should better describe what they are trying to achieve. Several comments about examples suggested incorrect changes, which signals that readers failed to understand them.
After a lot of consideration I decided against requesting the author to remove unique_resource and related functionality from the library. While several reviewers wanted a more "concise" library, all noted that the component is useful.
One last thing I wanted to point out is that all reviewers and most commenters were adamant that compatibility with the Technical Specification on which this library was initially based should not be a goal. In fact, Andrzej Krzemienski even noted that such compatibility has prevented him from adequately evaluating the library on its own merit. In my opinion this should be a sign to all of us that people prefer better APIs over standard APIs. I recommend Andrey to re-evaluate those parts of the library's APIs that he copied from the TS and potentially pick better names for member functions.
Thanks to Andrey for submitting this library to Boost. And thanks again to the reviewers and everyone who provided feedback, who ultimately made this review possible.
I have updated the library according to review comments and conditions. To specifically address the acceptance conditions: 1. Noexcept specifications for destructors are now present in the docs. For example: https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/defer_guard... https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/scope_exit.... https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/unique_reso... The fix for the BoostBook stylesheets is currently in develop. Hopefully, it will get merged to master soon. 2. I have added an example showing usage of all three conditional scope guards, including scope_success, here: https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html... The example is based on the one I posted during the review. 3. I have added more comments to some examples, as well as the discussion around them. For example, the examples on the front page are better commented now: https://lastique.github.io/scope/libs/scope/doc/html/index.html Some examples were modified or completely rewritten. For example, the locked_write_string was modified to reduce code duplication: https://lastique.github.io/scope/libs/scope/doc/html/index.html The leading example for unique_resource was rewritten: https://lastique.github.io/scope/libs/scope/doc/html/scope/unique_resource.h... And a new example for unique_resource was added on the front page: https://lastique.github.io/scope/libs/scope/doc/html/index.html There are many more additions and modifications than I can mention here. There were also changes that were requested by reviewers or were the result of the discussion. The changelog here lists most of the changes: https://lastique.github.io/scope/libs/scope/doc/html/scope/changelog.html To mention the most prominent changes: * BOOST_SCOPE_FINAL was renamed to BOOST_SCOPE_DEFER and scope_final to defer_guard. * The `release()` member of scope guards was removed. `set_active()` is still present and is the way to activate and deactivate scope guards. * Added `operator bool()` to `unique_resource`. * Added a C++17 `unallocated_resource` class template that simplifies declaration of resource traits when all unallocated resource values can be listed. Along with the recently added to Boost.Core `functor` template, this can significantly reduce the amount of code one needs to write to declare a `unique_resource`. For example, a `unique_resource` for Windows handles could be declared like this: unique_resource< HANDLE, functor< CloseHandle >, unallocated_resource< INVALID_HANDLE_VALUE, (HANDLE)NULL >
handle;
https://lastique.github.io/scope/libs/scope/doc/html/scope/unique_resource.h... https://www.boost.org/doc/libs/develop/libs/core/doc/html/core/functor.html * `unique_resource` now enforces requirements on the resource traits with static asserts rather than falling back to the "no resource traits" mode of operation. * `unique_resource` no longer supports the "reduced" resource traits, where the traits were only used to customize the default resource value but not to distinguish allocated and unallocated resource values. * Added documentation sections discussing differences with the Library Fundamentals TS: https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html... https://lastique.github.io/scope/libs/scope/doc/html/scope/unique_resource.h... * On the front page, removed comparison with Boost.ScopeExit from the table. There is a separate section for it: https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html... Also added another row to the table that shows explicit deactivation of the scope guards by calling `set_active(false)`. * There is now a section discussing caveats of capturing variables by reference in scope guards: https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html... There are probably other changes that I forgot to mention. Dmitry, please let me know if you consider the review conditions satisfied or if there needs to be done something more. If everything's fine, what needs to be done to move the library into boostorg?
participants (3)
-
Andrey Semashev -
Vinnie Falco -
Дмитрий Архипов