[scope] review

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2023-12-05 14:34:39

Hi,

This is my review of the proposed boost scope library.

* Does this library bring real benefit to C++ developers for real world 
use-case?

Yes. Scope guards are a well-known and widely used feature, nearly every 
code-base has some implementation of it. The same goes for raii resource 
wrappers.

* Do you have an application for this library?

I would consider using it for new projects. It does not, however, offer 
significant value at the time which would justify migrating from our 
homebrew scope guard. Worse, it misses some guarantees that makes it 
less fitted (but arguably it is more generic than what we use now).

* Does the API match with current best practices?

The API is copied from a TS proposal, for some parts. Most of my griefs 
against the API also applies to the TS.

While I was not fond of it at the start, i think there's a clear value 
in differentiating scope_success and scope_fail. The first one is 
allowed to throw, the second is not. This, however, is not a strong 
requirement, but a general advice. It thus raises the concern about why 
not simply giving a boolean parameter to the scope_guard handler 
function, and letting it act accordingly. I suggest making the 
noexcept-ness of scope_fail executor a strong requirement.

scope_success / scope_fail are not default-constructible, although they 
have a valid empty state. While there's a rationale behind this (it is a 
bit uncommon to create a scope guard that does nothing), it is a 
limitation for the sake of nothing. Initializing an empty scope_exit 
allows scope escaping (useful when initializing the scope guard in the 
body of an if statement). There will always be the possiblity to do it 
with an optional, so it just seems that forbidding default construction 
has no value.

To check whether a scope_xxx is active or not, one has to call the 
active() method. To check whether a unique_resource has a value, one has 
to call the allocated() method. These are additions compared to the TS. 
While they may be useful, the names are inconsistent, both are new, also 
inconsistent with what other nullable types (optional, unique_ptr, 
variant...) uses. While this is common practice accross the c++ 
standard, I would not called this *best*. This leads to problems for 
generic code, and proposals such as this one: 
https://herbsutter.com/2022/09/25/something-i-implemented-today-is-void/ 
. In both case, i think at least an operator bool() should be provided, 
because it's a more widespread and common way to check for emptiness / 
valid state.

scope_success / fails / exit takes an additional parameter (extension 
over TS), which is the execution condition. scope_success executes when 
the condition returns false. This is unintuitive, and something i will 
usually get wrong. While i get the reasoning behind, i think it tells 
more in favor of a scope_conditional<SuccessHandler, FailureHandler> 
interface. I'm not sure there's a lot of use-cases for a success_only or 
fail_only scope guard, especially since you can release in case of 
success, to avoid the cost of the failure check.

About this customizable failure detector, i'm not found of the idea. The 
default, detection of a current exception, is a sane one. And it allows 
us to require FailureHandler to be noexcept. That's no longer the case 
if you allow customizing the failure detector. I'm not sure it's worth 
the tradeoff, since the failure detection could always be done inside 
the body of a standard unconditional scope_exit.

Finally, i don't get the point of set_active. Sure, it's cheap to 
implement, but do we really want to encourage dynamically enabling scope 
guards ? Disabling a scope guard makes a lot of sense, that's what you 
do to disable a rollback scope_guard upon successful operation. 
Dynamically enabling it looks more like being hostile to the reader to 
me. In all cases, it's possible to get this behaviour with a boolean 
inside the scope guard callback, so this use case is already addressed. 
There's no need for it to be a first-class citizen.

I like the idea of the lighter, unconditional, scope_final, which is 
missing in the TS.

* Is the scope of the library well-defined?

unique_resource and scope_exit are quite different beasts. They only 
have in common being RAII. But that also include unique_lock, 
unique_ptr, and nobody thought about making them part of the same 
library. I think it is a mistake to mix them.

unique_resource takes three template parameters, the third one being a 
predicate telling if the resource is valid. I understand the wish to 
avoid storing an extra information, when there's a possibility to do 
without by storing an invalid value. But to me, it looks like a more 
widespread problem already solved, for example by 
https://github.com/akrzemi1/markable . It would be far better IMHO to 
get markable in boost, and have unique_resource specialized for 
markable<X> than replicating most of its functionality into 
unique_resource.

* Is the documentation helpful and clear?

The documentation is clear enough, although i could not find some 
information that i think should be emphasized more (ie, what the library 
guarantees in terms of pre / post conditions, noexceptness, etc). I 
don't like the way the reference section subitems are links to the file 
headers descriptions, and not links to class descriptions. I'd prefer 
something like:

Reference
   * template<  > class scope_exit
   * template<  > class scope_final
   * ...

instead of the headers, for which i usually don't care (i just need to 
know which header i need to include, and this information is provided in 
the class description).

There are a few examples in the documentation that can be improved. 
There's always room for improvement, the overall quality is good.

* Did you try to use it? What problems or surprises did you encounter? 
How much time did you spent?

I made some small experiments with it. I spent around 6-8 hours 
reviewing the library and writing this review. Most of the time was 
spent reading the documentation and checking the behaviour. No problems 
and no surprises so far.

* What is your evaluation of the implementation?

The library is big (~2,5k lines of code), given what it offers. Most of 
this comes from the fact that the library offers a lot of customization 
points, maybe too much. Others have pointed that it reimplements some 
standard features, for the sake of not including some headers. I don't 
like this choice, but I don't know what's boost policy in this regard 
is. In all cases, a reimplementation with a name is better than nothing. 
In particular, there are some static_cast<T&&> which would advantagely 
be replaced by a detail::forward<T>, the latter being far more readable 
than the former. It's a shame that compact_storage is not provided as a 
facility: i guess there are several similar implementation across boost. 
But that's not the library's fault.

Outside that, the organisation is clean, and the code largely readable, 
given that it needs to be compatible with C++11.

* What is you final evaluation of the library?

My current recommandation is to CONDITIONNALY ACCEPT the library. I 
think the following should be adressed, but i'd note that several of 
these comments also applies to the TS:

- scope_success and scope_fail should be unified into a single call, 
taking two callbacks. This will make a slightly bigger object, but if 
you plan on detecting exceptions then you don't mind the extra cost 
anyway. And it will allow to evaluate the condition only once, so that 
may prove a gain in several situations.
- scope_success and scope_fail should not take an execution condition. 
Exception detection should be enough for everyone, or else you should 
resort to scope_exit.
- the failure handler (scope_fail) should be enforced to be noexcept
- scope_exit should be default constructible (released by default, 
equivalent to moved-from state). That does not hold for scope_final, 
which is not releasable.
- the active / released name fiasco should be adressed. At least provide 
operator bool().
- set_active should be removed. This is encouraging bad practice.
- unique_resource should be moved outside the library. I consider that 
it belongs to a separate library, and i think it has some flaws in its 
design that should be addressed.

Finally, many thanks to Andrey for proposing this library, and for 
quickly replying to the questions during the review.

Regards,

Julien