From 7d49d4b45d2cc37171351268c6cce43c32dc28a0 Mon Sep 17 00:00:00 2001
From: Herb Sutter <herb.sutter@gmail.com>
Date: Mon, 10 Oct 2022 16:09:21 -0700
Subject: [PATCH] Clean up `final_act` and `finally`, closes #846 (#977)

Somewhere along the way, GSL's implementation of final_act and finally seems to have become way overthought. This PR is to re-simplify these facilities back to what C++ Core Guidelines C.30 said which is simple and clear and works. It just copies the invocable thing, and doesn't bother trying to optimize the copy. This should be fine, because we're typically passing something that's cheap to copy, often a stateless lambda.

The problem in #846 appears to be because finally looks like was originally written as a const&/&& overload (its state at the time that issue was opened)... to eliminate a copy when you invoke it with a temporary. If so, then the && was probably never intended to be a forwarder, but an rvalue reference that tripped over the horrid C++ syntax collision where a && parameter magically instead means a forwarding reference because the type happens to be a template parameter type here. So I suspect the original author was just trying to write an rvalue overload, and the forwarder that's there now was never intended at all.
---
 include/gsl/util      | 35 ++++++++++++-----------------------
 tests/utils_tests.cpp | 10 ++++++++++
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/include/gsl/util b/include/gsl/util
index aebfc66..75c78ad 100644
--- a/include/gsl/util
+++ b/include/gsl/util
@@ -65,40 +65,29 @@ template <class F>
 class final_action
 {
 public:
-    static_assert(!std::is_reference<F>::value && !std::is_const<F>::value &&
-                      !std::is_volatile<F>::value,
-                  "Final_action should store its callable by value");
+    explicit final_action(const F& ff) noexcept : f{ff} { }
+    explicit final_action(F&& ff) noexcept : f{std::move(ff)} { }
 
-    explicit final_action(F f) noexcept : f_(std::move(f)) {}
+    ~final_action() noexcept { if (invoke) f(); }
 
     final_action(final_action&& other) noexcept
-        : f_(std::move(other.f_)), invoke_(std::exchange(other.invoke_, false))
-    {}
+        : f(std::move(other.f)), invoke(std::exchange(other.invoke, false))
+    { }
 
-    final_action(const final_action&) = delete;
-    final_action& operator=(const final_action&) = delete;
-    final_action& operator=(final_action&&) = delete;
-
-    // clang-format off
-    GSL_SUPPRESS(f.6) // NO-FORMAT: attribute // terminate if throws
-    // clang-format on
-    ~final_action() noexcept
-    {
-        if (invoke_) f_();
-    }
+    final_action(const final_action&)   = delete;
+    void operator=(const final_action&) = delete;
+    void operator=(final_action&&)      = delete;
 
 private:
-    F f_;
-    bool invoke_{true};
+    F f;
+    bool invoke = true;
 };
 
 // finally() - convenience function to generate a final_action
 template <class F>
-GSL_NODISCARD final_action<typename std::remove_cv<typename std::remove_reference<F>::type>::type>
-finally(F&& f) noexcept
+GSL_NODISCARD auto finally(F&& f) noexcept
 {
-    return final_action<typename std::remove_cv<typename std::remove_reference<F>::type>::type>(
-        std::forward<F>(f));
+    return final_action<std::decay_t<F>>{std::forward<F>(f)};
 }
 
 // narrow_cast(): a searchable way to do narrowing casts of values
diff --git a/tests/utils_tests.cpp b/tests/utils_tests.cpp
index 0658263..5b64e20 100644
--- a/tests/utils_tests.cpp
+++ b/tests/utils_tests.cpp
@@ -112,6 +112,16 @@ TEST(utils_tests, finally_function_ptr)
     EXPECT_TRUE(j == 1);
 }
 
+TEST(utils_tests, finally_function)
+{
+    j = 0;
+    {
+        auto _ = finally(g);
+        EXPECT_TRUE(j == 0);
+    }
+    EXPECT_TRUE(j == 1);
+}
+
 TEST(utils_tests, narrow_cast)
 {
     int n = 120;