Re: [scope] Scope review starts on November 26th

From: Janko Dedic (jankodedic2_at_[hidden])
Date: 2023-11-29 10:14:44

Thanks for proposing this library. Scope guards are a natural and much
needed addition to Boost. The following is my review of the proposed
Boost.Scope.

The first thing I looked for in this library was a convenience SCOPE_EXIT {
... }; macro that would provide the functionality in the form of a control
flow construct. I've managed to quickly find it, but it wasn't exactly what
I was expecting.

BOOST_SCOPE_FINAL []
{
    std::cout << "Hello world!" << std::endl;
};

I found it rather odd that the capture list is not included in the macro. I
looked a bit further into the documentation to find this:

BOOST_SCOPE_FINAL std::ref(cleanup_func);

Is this use case the reason why the capture list is required? It seems like
a very rare one that shouldn't pessimize the interface for 99% of use
cases. Performance shouldn't be a concern because the compiler sees all
this code and inlines it anyway. In my view, this just makes the interface
worse and less pleasant to use.

scope_success doesn't seem terribly useful. I have yet to see a legitimate
use case of this type. I haven't found a motivating example in the
documentation. There should either be one, or this type should be removed
from the library.

There is no BOOST_SCOPE_FAIL macro. Why is it missing?

I found the scope_fail example pretty bad:

class collection
{
    std::set< std::shared_ptr< object > > objects;

public:
    void add_object(std::shared_ptr< object > const& obj)
    {
        // Create a deactivated scope guard initially
        std::set< std::shared_ptr< object > >::iterator it;
        boost::scope::scope_fail rollback_guard{[&, this]
        {
            objects.erase(it);
        },
        false};

        bool inserted;
        std::tie(it, inserted) = objects.insert(obj);
        if (inserted)
        {
            // Activate rollback guard
            rollback_guard.set_active(true);
        }

        obj->on_added_to_collection(*this);
    }
};

It could be simply rewritten to not use set_active (which should be
discouraged, because it makes code less declarative and harder to follow):

class collection
{
    std::set< std::shared_ptr< object > > objects;

public:
    void add_object(std::shared_ptr< object > const& obj)
    {
        // Create a deactivated scope guard initially
        std::set< std::shared_ptr< object > >::iterator it;

        bool inserted;
        std::tie(it, inserted) = objects.insert(obj);
        if (inserted)
        {
            boost::scope::scope_fail rollback_guard{[&, this]
            {
                objects.erase(it);
            }};
            obj->on_added_to_collection(*this);
        }
    }
};

Could be made even cleaner by using an early return: if (!inserted) return;

All destructor declarations in the documentation lack noexcept
specification, which by default means that they're noexcept, but they're
actually not if you look at the Throws section, or the implementation.

Also, the question presents itself: why are these destructors allowed to
throw? I feel like it is common knowledge today that destructors should not
throw, and this is heavily frowned upon. I can see the want to "be generic"
here, but I don't see why a library should entertain broken code.

I also noticed that certain scope guards are move constructible. Why? I
cannot imagine myself ever approving of someone moving a scope guard on
code review. Scope guards are to be used exclusively as local variables and
don't need to be moved. This just opens up room for misuse.

I don't see any compelling use cases for scope_exit::set_active in the
documentation. The only example I find motivating is this one:

void push_back(int x, std::vector<int>& vec1,
    std::vector<int>& vec2)
{
    vec1.push_back(x);

    // Revert vec1 modification on failure
    boost::scope::scope_exit rollback_guard{[&]
    {
        vec1.pop_back();
    }};

    vec2.push_back(x);

    // Commit vec1 modification
    rollback_guard.set_active(false);
}

This could as well have used rollback_guard.release();. This matches my
personal experience: only dismiss/release/cancel is necessary for real use
cases ("commiting" something and thus disabling the "rollback" action).
absl::Cleanup for example, supports only Cancel(). There is value in
preventing users from shooting themselves in the foot.

I don't like the name scope_exit::release personally. I feel like "dismiss"
or "cancel" would be much better. I know that std::experimental::scope_exit
calls it "release", but we shouldn't appeal to false authority.

I did not like the locked_write_string example. Reading this code locally:

    // Lock the file
    while (flock(fd, LOCK_EX) < 0)
    {
        err = errno;
        if (err != EINTR)
            return;
    }

it's hard to get what's happening, why we're setting err and how big of an
impact that has on function behavior. A better way to write this code would
be to simply extract this piece into its own function:

        err = errno;
        if (err != EINTR)
            return;

together with the ec = std::error_code(err, std::generic_category()), which
doesn't require a scope guard.

fd_deleter and unique_fd provide some nice handling for EINTR, which wasn't
required but I think it's nice to have and it's good that it's a part of
this library!

I think make_unique_resource_checked approaches the problem from the wrong
direction. This code:

    // Create a unique resource for a file
    auto file = boost::scope::make_unique_resource_checked(
        open(filename.c_str(), O_CREAT | O_WRONLY)
        -1,
        fd_deleter());
    if (!file.allocated())
    {
        int err = errno;
        throw std::system_error(err, std::generic_category(), "Failed to
open file " + filename);
    }

could be this:

    // Create a unique resource for a file
    int file =  open(filename.c_str(), O_CREAT | O_WRONLY);
    boost::scope::scope_final close_file(fd_deleter{});
    if (file != -1)
    {
        int err = errno;
        throw std::system_error(err, std::generic_category(), "Failed to
open file " + filename);
    }

This has the advantage of being able to use `file` directly for the rest of
the code instead of `file.get()`.

I think a similar utility would be more useful in form of a class template,
for example:

resource<HANDLE, INVALID_HANDLE_VALUE, ::CloseHandle> windows_handle;
resource<SDL_Window*, nullptr, ::SDL_DestroyWindow> sdl_window_handle;

This would be very useful for concisely implementing RAII wrappers over C
handles (you don't have to manually write destructors, move constructors
and move assignment operators). For local use (within a function), I feel
like a local handle + scope guard is just more ergonomic than the whole
make_unique_resource_checked thing.

My overall impression is that this library is trying to be too generic when
it's not really necessary. There's definitely useful stuff here that I want
to see included, but also a lot of stuff that is too much. I don't
personally see the need for any scope guard beyond scope_exit with a
release. That covers all the use cases I've ran into very nicely.
scope_fail can be more error-prone since the "commit" is not explicit, so
you may end up doing rollback even when it's unnecessary. Using the
modified example from the documentation:

void push_back(int x, std::vector<int>& vec1,
    std::vector<int>& vec2)
{
    vec1.push_back(x);

    // Revert vec1 modification on failure
    boost::scope::scope_fail rollback_guard{[&]
    {
        vec1.pop_back();
    }};

    vec2.push_back(x);
    do_something_else(); // if this throws, rollback_guard runs
}

If you use scope_exit with an explicit release, it's clearer IMO:

void push_back(int x, std::vector<int>& vec1,
    std::vector<int>& vec2)
{
    vec1.push_back(x);

    // Revert vec1 modification on failure
    boost::scope::scope_exit rollback_guard{[&]
    {
        vec1.pop_back();
    }};

    vec2.push_back(x);
    rollback_guard.release(); // commit
    do_something_else(); // if this throws, rollback_guard won't run
}

The documentation states that "exception_checker is incompatible with C++20
coroutines", which is true in general, but it doesn't have to be (depending
on the coroutine library implementation). However, I bring this up because
using scope_exit+release is an entirely viable alternative for coroutines.
scope_exit+release will work whether you use exceptions, error codes,
std::expected, coroutines or all of them combined. This is the beauty of
it, which makes me feel like scope_fail is unnecessary.

I vote to ACCEPT this library with the following CONDITIONS:
- BOOST_SCOPE_FINAL includes the lambda capture list OR another macro that
does is provided;
- noexcept specification on destructors is properly documented;
- move constructors on all scope guard types are deleted;
- a motivating example for scope_success is provided, or the type is
removed.

These are my conditions, but I'm hoping the full review is taken as a
recommendation in improving the library and the documentation.

Best regards,
Janko Dedic