Re: [Scope] Peer review conclusion

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2024-02-05 00:47:37

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?