Re: [Scope] Peer review conclusion

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2024-02-06 19:33:34

On 2/6/24 22:29, Дмитрий Архипов wrote:
> Looks good to me. I consider the review conditions satisfied.

Thank you Dmitry. And especially for taking the time to review the
documentation and point out errors.

> пн, 5 февр. 2024 г. в 03:47, Andrey Semashev <andrey.semashev_at_[hidden]>:
> 
>>
>> 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.html
>> https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/scope_exit.html
>> https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/unique_resource.html
>>
>> 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#scope.scope_guards.conditional
>>
>> 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.html
>>
>> 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.html#scope.unique_resource.simplified_resource_traits
>> 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#scope.scope_guards.comparison_with_library_fundamentals_ts
>> https://lastique.github.io/scope/libs/scope/doc/html/scope/unique_resource.html#scope.unique_resource.comparison_with_library_fundamentals_ts
>>
>> * 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#scope.scope_guards.comparison_with_boost_scope_exit
>>
>> 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#scope.scope_guards.capture_by_reference_caveats
>>
>> 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?
>>