Add scope_exit utilities #3840

HowardHinnant · 2021-05-06T19:11:06Z

High Level Overview of Change

Add scope_exit utilities from Library Fundamental, Version 3

Basic design of idea: https://www.youtube.com/watch?v=WjTrfoiB0MQ
Specification:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/n4873.html#scopeguard

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Test Plan

Unit tests added.

seelabs

👍 Looks great! Left some minor comments, but nothing that should hold this up. Thanks for coding this up; as you know I'll have a good place to use it already.

seelabs · 2021-05-06T20:01:26Z

src/ripple/basics/scope.h

+    scope_exit(scope_exit&& rhs) noexcept(
+        std::is_nothrow_move_constructible_v<EF> ||
+        std::is_nothrow_copy_constructible_v<EF>)
+        : exit_function_{std::forward<EF>(rhs.exit_function_)}


Since rhs is an rvalue, not a universal ref, shouldn't this be std::move, not std::forward?

forward is used because EF is allowed to be an lvalue reference to a functor. So this is a move if EF is an object, and a reference binding if EF is an lvalue reference. With std::move this would fail to compile if EF is a lvalue reference.

Makes sense. Thanks for the explanation.

seelabs · 2021-05-06T20:02:28Z

src/ripple/basics/scope.h

+        std::enable_if_t<
+            !std::is_same_v<std::remove_cv_t<EFP>, scope_exit> &&
+            std::is_constructible_v<EF, EFP>>* =
+            0) noexcept(std::is_nothrow_constructible_v<EF, EFP> || std::is_nothrow_constructible_v<EF, EFP&>)


It's a shame the enable_if has to be a parameter - which hides it away from where we see it in the rest of the rippled code. But I think we're forced to do that because this is a constructor.

Right. And requires isn't in our toolbox yet. (requires is going to be great...)

seelabs · 2021-05-06T20:04:56Z

src/ripple/basics/scope.h

+    try : exit_function_
+    {
+        std::forward<EFP>(f)
+    }


I would have left the try block off myself (which really means leaving off this whole overload). The idea that the move constructor for the function would throw just isn't going to happen in rippled code. But I know this is meant to mimic standard library code, not ripple code. No change needed. I'll also note I think this is the first time I've seen function try block used in practice.

It's also the first time I've used a function try block. :-)

scottschurr · 2021-05-07T18:51:01Z

It looks like all of the gcc builds are failing CI. I didn't look at them all, but one of the failing gcc builds says this:

1567 In file included from /home/travis/build/ripple/rippled/src/test/basics/scope_test.cpp:20,
1568                  from CMakeFiles/rippled.dir/Unity/unity_40.cxx:6:
1569 ../../src/ripple/basics/scope.h: In instantiation of 'ripple::scope_exit<EF>::scope_exit(EFP&&, std::enable_if_t<((! is_same_v<typename std::remove_cv<_BoundArg>::type, ripple::scope_exit<EF> >) && is_constructible_v<EF, EFP>)>*) [with EFP = ripple::test::scope_test::test_scope_exit()::<lambda()>; EF = ripple::test::scope_test::test_scope_exit()::<lambda()>; std::enable_if_t<((! is_same_v<typename std::remove_cv<_BoundArg>::type, ripple::scope_exit<EF> >) && is_constructible_v<EF, EFP>)> = void]':
1570 /home/travis/build/ripple/rippled/src/test/basics/scope_test.cpp:35:43:   required from here
1571 ../../src/ripple/basics/scope.h:75:9: error: throw will always call terminate() [-Werror=terminate]
1572          throw;
1573          ^~~~~

A quick scan of some of the other gcc builds showed a similar complaint.

The Windows builds show a similar complaint:

552 ..\src\ripple/basics/scope.h(75): error C2220: warning treated as error - no 'object' file generated
553 ..\src\ripple/basics/scope.h(75): warning C4297: 'ripple::scope_exit<ripple::test::scope_test::test_scope_exit::<lambda_9e24fdaf89f344e157db56bfc6f19973>>::scope_exit': function assumed not to throw an exception but does
554 ..\src\ripple/basics/scope.h(75): note: __declspec(nothrow), throw(), noexcept(true), or noexcept was specified on the function
555 C:/Users/travis/build/ripple/rippled/src/test/basics/scope_test.cpp(35): note: see reference to function template instantiation 'ripple::scope_exit<ripple::test::scope_test::test_scope_exit::<lambda_9e24fdaf89f344e157db56bfc6f19973>>::scope_exit<ripple::test::scope_test::test_scope_exit::<lambda_9e24fdaf89f344e157db56bfc6f19973>>(EFP &&,void *) noexcept' being compiled
556         with
557         [
558             EFP=ripple::test::scope_test::test_scope_exit::<lambda_9e24fdaf89f344e157db56bfc6f19973>
559         ]

HowardHinnant · 2021-05-07T19:17:32Z

Ah. Yes, when the functor construction can't throw an exception the function try catch is superfluous. And instead of optimizing it away the compiler is whining about it. :-\

The only way I see to fix this is to create two constructors: one constrained when noexcept is false and the other when noexcept is true, and only put the try/catch on the noexcept false version. And that's just ugly enough for me to balk.

I think instead the best path forward is to eliminate the try/catch, and static_assert that the construction is noexcept. I'll test that a bit before I push it.

of scope_exit and scope_fail.

HowardHinnant · 2021-05-07T19:47:26Z

Ok, this is working on clang. This means (for example), one can not construct scope_exit with an lvalue std::function because that might throw (error caught at compile-time). But you can construct from an rvalue std::function. Though it would be poor form to construct the std::function within the scope of the handled resource, because that construction can throw too.

Best use case is to use a lambda that has a noexcept constructor and move constructor.

scottschurr

👍 Looks great. I'm doubtful that anyone at Ripple will use scope_fail or scope_success, but since we're exercising things for the committee I think it's okay to include them.

I left a few notes for you to consider on the unit tests. But they are at your discretion. Thanks for doing this!

scottschurr · 2021-05-07T19:50:22Z

src/ripple/basics/scope.h

+// permitted to throw an exception.  This was done because some compilers
+// did not like the superfluous try/catch in the common instantiations
+// where the construction was noexcept.  Instead a static_assert is used
+// to enforce this restriction.


Nice comment. Thanks.

scottschurr · 2021-05-07T22:23:56Z

src/test/basics/scope_test.cpp

+        }
+        BEAST_EXPECT(i == 1);
+        {
+            scope_exit x{[&i]() { i = 3; }};


What would you think of changing this test to i += 2? That would demonstrate that exactly one of the two scope_exit objects executed.

scottschurr · 2021-05-07T23:25:32Z

src/test/basics/scope_test.cpp

+BEAST_DEFINE_TESTSUITE(scope, ripple_basics, ripple);
+
+}  // namespace test
+}  // namespace ripple


There's no newline at the end of scope_test.cpp. Might be nice to add one.

scottschurr · 2021-05-07T23:33:29Z

src/test/basics/scope_test.cpp

+            }
+        }
+        BEAST_EXPECT(i == 5);
+    }


Oops. One other note that I'd apparently failed to save. All of the tests currently use lambdas. What would you think of testing a bog standard function as well? Perhaps something like...

struct scope_test : beast::unit_test::suite { inline static int testVal = 0; // static function to execute in tests static void scopeFunc() { testVal = 10; } ... testVal = 0; { scope_exit x{scopeFunc}; } BEAST_EXPECT(testVal == 10);

That works, BTW. It just seems like we should exercise it in the unit tests. Your call...

scottschurr

👍

Add scope_exit utilities

a6f61bb

HowardHinnant requested review from thejohnfreeman, seelabs and scottschurr May 6, 2021 19:11

HowardHinnant assigned thejohnfreeman, seelabs and scottschurr May 6, 2021

seelabs approved these changes May 6, 2021

View reviewed changes

[FOLD] Disallow exceptions in the constructors

e2e702c

of scope_exit and scope_fail.

scottschurr approved these changes May 7, 2021

View reviewed changes

scottschurr reviewed May 7, 2021

View reviewed changes

[FOLD] Minor fixes in test

6f7076b

scottschurr approved these changes May 12, 2021

View reviewed changes

thejohnfreeman approved these changes May 13, 2021

View reviewed changes

HowardHinnant added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 13, 2021

ximinez mentioned this pull request May 14, 2021

CheckCashMakesTrustLine amendment #3823

Closed

3 tasks

This was referenced Jun 2, 2021

Proposed 1.8.0-b3 #3857

Closed

Proposed 1.8.0-b3 #3858

Merged

manojsdoshi closed this in #3858 Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scope_exit utilities #3840

Add scope_exit utilities #3840

HowardHinnant commented May 6, 2021

seelabs left a comment

seelabs May 6, 2021

HowardHinnant May 6, 2021

seelabs May 6, 2021

seelabs May 6, 2021

HowardHinnant May 6, 2021

seelabs May 6, 2021 •

edited

Loading

HowardHinnant May 6, 2021

scottschurr commented May 7, 2021

HowardHinnant commented May 7, 2021

HowardHinnant commented May 7, 2021 •

edited

Loading

scottschurr left a comment

scottschurr May 7, 2021

scottschurr May 7, 2021

scottschurr May 7, 2021

scottschurr May 7, 2021

scottschurr left a comment

Add scope_exit utilities #3840

Add scope_exit utilities #3840

Conversation

HowardHinnant commented May 6, 2021

High Level Overview of Change

Type of Change

Test Plan

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr commented May 7, 2021

HowardHinnant commented May 7, 2021

HowardHinnant commented May 7, 2021 • edited Loading

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

seelabs May 6, 2021 •

edited

Loading

HowardHinnant commented May 7, 2021 •

edited

Loading