Andrey Semashev wrote:

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.

Other languages call this "defer". E.g. defer fclose( fp ); or defer { fclose( fp ); }

On 11/30/23 00:35, Janko Dedic via Boost wrote:

On Wed, Nov 29, 2023 at 4:56 PM Andrey Semashev via Boost < boost@lists.boost.org> 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.

Better in what way, other than your personal preference? Can you describe objective advantages of your proposed syntax and why the current BOOST_SCOPE_FINAL doesn't work for you?

...

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 [&].

Sorry, but no. That would limit user's options for no reason, and binding variables by value is not uncommon. Users of Boost.Scope should have the full range of function object definition ways that is permitted by the language.

...

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.

I had something like this in mind: void foo() { auto private_data = do_something(); scope_success commit_guard{[&private_data] { commit_data(private_data); }}; if (condition1) { do_something_on_condition1(private_data); return; } if (condition2) { do_something_on_condition2(private_data); return; } bool succeeded = complete_processing(private_data); if (!succeeded) { commit_guard.set_active(false); return; } } In the above pseudocode, private_data may be any number of data items of various types, including those defined locally in foo(), and function calls like commit_data, do_something_on_condition1, etc. would be pieces of code working on that and other data.

...

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

If you read earlier commits in this review, you'll see that there are people who would prefer Boost.Scope to *exactly* reflect the TS with no extensions or deviations. Although I disagree with that point of view, I still think there is value in providing interface that is *compatible* with the TS, if possible, so that users, at least potentially, have the option to switch between Boost.Scope and std, should they need to for whatever reason. Another reason is that this compatibility reduces the element of surprise and makes learning the library (whether Boost.Scope or std) easier. If this review shows that no such compatibility is desired by the community, I can remove some elements of the library that are deemed unwanted or detrimental to its quality.

...

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 see nothing wrong with this code. I actually wrote code like this, where the no-throw guarantee was important.

...

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

Reference lifetime extension has nothing to do with it. Without RVO, in order to return the scope guard from the factory function by value, its copy or move constructor needs to be invoked. Besides, TS scope guards are move-constructible.

...

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 can update the example in the docs.

...

...

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 don't see how. Can you provide a code example? Note that the checks for EINTR are made in different contexts. Some of them cause return, other - restart loop iteration, third - affect error_code initialization.

...

...

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

resource windows_handle; resource 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.

This would also prohibit resource types with non-constexpr constructors, as well as non-default-constructible resources. I don't like this tradeoff.

On 11/30/23 19:02, Peter Dimov via Boost wrote:

Andrey Semashev wrote:

...

...

...

...

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

resource windows_handle; resource sdl_window_handle;

That's

template class resource;

...

This would also prohibit resource types with non-constexpr constructors, as well as non-default-constructible resources. I don't like this tradeoff.

It doesn't. The two operations required from R are construction from Def and comparing to Def.

https://godbolt.org/z/h7z3qGzvs

Note that the type of Def doesn't have to be R.

What I meant is something like this won't work: https://godbolt.org/z/hczoP4WMx And if you're suggesting to define some magic placeholder type that can be used as Def and automatically converts to R then that looks like a more contrived usage to me than just defining resource traits.

On 11/30/23 22:37, Peter Dimov via Boost wrote:

Andrey Semashev wrote:

...

...

That's

template class resource;

...

This would also prohibit resource types with non-constexpr constructors, as well as non-default-constructible resources. I don't like this tradeoff.

It doesn't. The two operations required from R are construction from Def and comparing to Def.

https://godbolt.org/z/h7z3qGzvs

Note that the type of Def doesn't have to be R.

What I meant is something like this won't work:

https://godbolt.org/z/hczoP4WMx

That's a common objection but the fix is simple:

https://godbolt.org/z/KTM9eb5r8

...

And if you're suggesting to define some magic placeholder type that can be used as Def and automatically converts to R then that looks like a more contrived usage to me than just defining resource traits.

That's typically not that hard either, although for std::string specifically it doesn't work very well because operator!= is templated. So we're hitting worst case:

constexpr struct invalid_t { operator std::string() const noexcept { return "invalid"; }

Not noexcept, std::string constructor may throw.

friend bool operator!=( std::string const& s, invalid_t ) { return s != "invalid"; } } invalid;

Most of the time it's much simpler, though.

Just for reference, here's how the current implementation would look: struct string_resource_traits { static std::string const& make_default() noexcept { return g_invalid; } static bool is_allocated(std::string const& res) noexcept { return res != g_invalid; } static const std::string g_invalid; }; const std::string string_resource_traits::g_invalid{ "invalid" }; struct string_deleter { void operator()(std::string const& res) noexcept { close(res); } }; using fd = unique_resource< std::string, string_deleter, string_resource_traits

;

https://godbolt.org/z/s4YTnbc5v

On Thu, Nov 30, 2023 at 4:52 PM Andrey Semashev via Boost < boost@lists.boost.org> wrote:

On 11/30/23 00:35, Janko Dedic via Boost wrote:

...

On Wed, Nov 29, 2023 at 4:56 PM Andrey Semashev via Boost < boost@lists.boost.org> 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.

Better in what way, other than your personal preference? Can you describe objective advantages of your proposed syntax and why the current BOOST_SCOPE_FINAL doesn't work for you?

Better because the user is not forced to type the capture list everywhere. Ideally we would not even require a semicolon at the end, but we can't get rid of that. The ideal is a core language construct like Go's defer or D's scope(exit).

...

...

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 [&].

Sorry, but no. That would limit user's options for no reason, and binding variables by value is not uncommon. Users of Boost.Scope should have the full range of function object definition ways that is permitted by the language.

And why is having these options useful? Note that the user can still use the constructor or the factory function directly to fully customize the behavior (if they want to for whatever reason). I can understand your motivation behind this macro (declaring an anonymous scope guard variable only), but in 99% of cases I would personally end up using BOOST_SCOPE_FINAL [&], and I don't understand why the library wouldn't provide such a macro (Alexandrescu's original SCOPE_EXIT works like this).

...

...

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 see nothing wrong with this code. I actually wrote code like this, where the no-throw guarantee was important.

It just looks like a weird macro incantation to me personally. SCOPE_EXIT { run(); }; at least tried to model a control flow construct, even though there's the unfortunate semicolon at the end. I strive to have the least amount of clever macros possible in my code.

...

...

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

Reference lifetime extension has nothing to do with it. Without RVO, in order to return the scope guard from the factory function by value, its copy or move constructor needs to be invoked.

Move constructor can be private to achieve this. Then, lifetime extension works and the user can't move on their side.

...

...

...

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 don't see how. Can you provide a code example?

Note that the checks for EINTR are made in different contexts. Some of them cause return, other - restart loop iteration, third - affect error_code initialization.

As far as I can tell, they're all the same. The last loop can be rewritten to be like the rest.

...

...

...

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

resource windows_handle; resource 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.

This would also prohibit resource types with non-constexpr constructors, as well as non-default-constructible resources. I don't like this tradeoff.

It's not necessarily a "trade-off." I personally don't find the full flexibility of unique_resource that useful, but I can understand why you'd want to support it. I'm just saying that the work you have to do by providing custom resource traits basically ends up being more than writing destructors and moves manually. I'm also suggesting that perhaps having a type like I've described that solves 99% of cases in one line might be a valuable addition to the library.

Andrey Semashev wrote:

If typing [&] is a problem, you could just

#define SCOPE_GUARD BOOST_SCOPE_FINAL [&]

and use that. Personally, I have no problem with the capture list, and quite often I actually use capture lists more elaborate than [&].

I _think_ that it might be possible to make the capture list optional by using BOOST_SCOPE_FINAL { }; and BOOST_SCOPE_FINAL(&) { }; but I'm not 100% sure. (And maybe rename it to BOOST_SCOPE_DEFER.)

On 12/2/23 17:30, Peter Dimov via Boost wrote:

(And maybe rename it to BOOST_SCOPE_DEFER.)

Would you also propose to rename the scope_final class template? What name would you suggest? Interestingly, there seems to be a proposal to add `defer` to C: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2895.htm So I guess, we should avoid taking the word `defer` in case it becomes a keyword some day. I'm not opposed to renaming, as long as the new name is short and not misleading. I'm not familiar with Go (presumably, from which the name comes), so I can't tell how similar the semantics of the feature in Go is to the proposed library. FWIW, in BOOST_SCOPE_FINAL, "final" is partly a reference to `__try`/`__finally` in MSVC and `try`/`finally` in Java, and partly emphasizes that the scope guard is "final" after construction, i.e. cannot be modified or cancelled. It also reads almost like "finale of the scope". I like it.

Andrey Semashev wrote:

Interestingly, there seems to be a proposal to add `defer` to C:

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2895.htm

Interestingly, the proposed `defer` is literally your BOOST_SCOPE_FINAL, complete with the mandatory [&]. So this is both an argument for retaining the explicit [&] and an argument in favor of renaming it to BOOST_SCOPE_DEFER. It's not clear what are the chances of N2895 in WG14, of course.