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

From: Janko Dedic (jankodedic2_at_[hidden])
Date: 2023-11-29 21:35:21

On Wed, Nov 29, 2023 at 4:56 PM Andrey Semashev via Boost <
boost_at_[hidden]> wrote:

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

It offers better syntax.

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

I only suggested adding [&] to the macro, or adding a macro that includes
the [&].

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

Do you have a code example? I'm really curious about this use case.

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

I very much dislike this reasoning. Why should Boost copy std even when std
is making a mistake? Boost should rely on its review process to judge
libraries. "std does the same" is not a valid argument (especially in the
light of recent discussions on the list about the quality of the WG21
"review process"). I can understand if it's the stated goal of the library
to copy std, but (1) scope guards are not even standardized yet and (2) for
copying std there is Boost.Compat.

At the end of the day, users are robbed of better libraries because "std
has already done it this way."

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

Even when you know that no other exception is in flight (scope_success), I
would consider code that throws from the scope guard highly dubious.

> 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 */ };
>

Does anyone really want to write code like this?

> > 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 can see why. C++17 fixes this "defect". You can technically rely on
reference lifetime extension, but that doesn't look as good.

auto&& guard = make_scope_exit(...);

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

This looks much better honestly. Why isn't this variant used for the
example?

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

It helps with all the return points in the example function.

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

 The advantage is doing the same thing in 1 line instead of 20 lines of
code, at which point writing the destructor and moves manually has a better
ROI than implementing traits classes.