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

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-11-29 15:56:14

On 11/29/23 13:14, Janko Dedic via Boost wrote:
> 
> 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.

You can think of BOOST_SCOPE_FINAL as a keyword that begins the
declaration of a scope guard. The scope guard action then is what
follows this keyword, and indeed it is not limited to just lambda
functions. I find this a useful ability, as this allows to reuse code in
user-defined function objects. In the example you quoted above,
cleanup_func could be written once and then used in multiple scope
guards without having to wrap each of them in a useless lambda.

I don't see the current definition of BOOST_SCOPE_FINAL as a deficiency
of the interface compared to passing the capture list in the macro
arguments. The latter doesn't offer any advantages compared to the
current definition, and in fact is more limiting as it only allows for
lambda functions for the scope guard action.

Besides, note that the lambda definition syntax has evolved over time
and may now include additional elements such as template parameters,
noexcept and mutable qualifiers and the trailing return type. There may
be new syntax additions in the future, which could be incompatible with
the macro definition. Although some of the syntax elements are useless
for the purpose of scope guards, at least noexcept and mutable are
meaningful and would look awkward if BOOST_SCOPE_FINAL accepted the
capture list in its parameters.

> 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.

scope_success isn't as widely useful as e.g. scope_fail, but you do
occasionally want it to schedule some commit steps when the enclosing
function succeeds, if there are multiple exit points and a lot of the
function-local state that is difficult or inconvenient to pass to a
separate function. You can sometimes work around it with a goto, and
scope_success is just another, possibly cleaner way to do this.

Also, and this pertains to some other questions you asked in your
review, Boost.Scope tries to provide components that are defined in the
Library Fundamentals TS with a compatible interface. scope_success is
defined in the TS, so Boost.Scope also provides it.

> There is no BOOST_SCOPE_FAIL macro. Why is it missing?

If you mean BOOST_SCOPE_FAIL to be equivalent to BOOST_SCOPE_FINAL but
only creating scope_fail then I didn't see enough motivation to add it.
scope_exit/fail/success support scope guard cancellation and also a
condition function object. This means users these scope guard types
often want to interact with the scope guard object (to
activate/deactivate it), and also that the scope guard construction may
be more elaborate.

Note that scope_exit/fail/success constructors are explicit and may
accept multiple arguments. This makes them incompatible with the syntax
used by BOOST_SCOPE_FAIL.

> 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;

Your rewritten code is not equivalent to the original, as it doesn't
call on_added_to_collection if inserted is false. The whole point of the
example is to show that the scope guard can be used to *conditionally*
invoke its action in case of failure.

Some examples in the docs could probably be written differently. Their
point is to demonstrate how a feature could be used in a certain very
simplified context.

> 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.

Unfortunately, this is how Doxygen works, I can't do anything about it.
The noexcept specification is present in the code.

> 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.

It is discouraged to throw from a destructor because it can cause
program termination, if the destructor is called during another
exception propagation. If your code knows that this can't happen (e.g.
in a scope_success, or a different scope guard, where you know in
runtime that the action may only throw if there is no other exception in
flight), throwing from a destructor is not a problem, as long as the
destructor is marked with noexcept(false). Which it is, for scope guards.

If you want to enforce that your action never throws, you can declare
its operator() as noexcept:

  BOOST_SCOPE_FINAL []() noexcept { /* I guarantee I won't throw */ };

> 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.

Movability is necessary for the factory functions, at the very least.

> 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.

You can call release(), if you prefer, it's equivalent to
set_active(false). You want set_active to be able to activate an
inactive scope guard. One example of that you have quoted yourself above.

> 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.

Yes, I'm not fond of release() either, but I'll still provide it for
compatibility with the TS. My preferred alternative is set_active().

And I do specifically prefer set_active() over a pair of methods like
activate()/deactivate() because passing a bool is fundamentally more
flexible than having two distinct methods. You can receive that bool
from elsewhere and just forward to the method instead of testing it
yourself. Like in the example above, you could have written it like this:

  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);

    // Activate rollback guard, if needed
    rollback_guard.set_active(inserted);

    obj->on_added_to_collection(*this);
  }

which wouldn't be as simple with two different methods.

> 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.

Sorry, I don't understand your suggestion. How would this help with
other return points of the function?

> 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()`.

The rewritten piece of code is incorrect and is exactly why
make_unique_resource_checked exists. Besides not binding the fd to
invoke fd_deleter on, it creates the scope guard before checking the fd
for validity, which means it will call close(-1) if open() fails.

You could modify the code to fix those issues, but you're still missing
the point of unique_resource, which is to bundle the resource with its
deleter. Similarly to how you bundle a pointer with its deleter by using
a unique_ptr.

As I said earlier in the discussion, I'm also not fond of
make_unique_resource_checked as I think the necessity to use this helper
is different from most other facilities and unnecessarily verbose. I
think, resource traits that are supported by Boost.Scope's
unique_resource are a better solution, as they not only eliminate the
need to use this helper, but they also allow for a more optimal
implementation of unique_resource. But I still provide
make_unique_resource_checked for compatibility with the TS.

> 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).

This would require C++17, if I'm not mistaken (or when were the auto
non-type template parameters introduced?). I don't think it offers much
advantage compared to the current unique_resource with resource traits.