Remove C++11 style factory function in absl::Cleanup

absl/cleanup/cleanup.h

@@ -16,58 +16,81 @@
 // File: cleanup.h
 // -----------------------------------------------------------------------------
 //
-// `absl::Cleanup` implements the scope guard idiom, invoking the contained
-// callback's `operator()() &&` on scope exit.
+// `absl::Cleanup` implements the scope guard idiom. On scope exit, it invokes
+// the contained callback via `std::move(callback)();`.
 //
-// This class doesn't allocate or take any locks, and is safe to use in a signal
-// handler. Of course the callback with which it is constructed also must be
-// signal safe in order for this to be useful.
+// By design, the implementation does not allocate, take any locks or otherwise
+// acquire resources. It is safe to use `absl::Cleanup` in a signal
+// handler, assuming the underlying callback is itself signal safe.
 //
 // Example:
 //
 // ```
-//   absl::Status CopyGoodData(const char* source_path, const char* sink_path) {
-//     FILE* source_file = fopen(source_path, "r");
-//     if (source_file == nullptr) {
-//       return absl::NotFoundError("No source file");  // No cleanups execute
+//   absl::Status CopyGoodData(const std::string& src_path,
+//                             const std::string& sink_path) {
+//     std::FILE* src_file = std::fopen(src_path.c_str(), "r");
+//     if (src_file == nullptr) {
+//       absl::Status result = absl::NotFoundError("No source file");
+//       return result;  // Zero cleanups execute
 //     }
 //
-//     // C++17 style cleanup using class template argument deduction
-//     absl::Cleanup source_closer = [source_file] { fclose(source_file); };
+//     // Ensure the source file is closed on scope exit...
+//     absl::Cleanup src_closer = [src_file] { std::fclose(src_file); };
 //
-//     FILE* sink_file = fopen(sink_path, "w");
+//     std::FILE* sink_file = std::fopen(sink_path.c_str(), "w");
 //     if (sink_file == nullptr) {
-//       return absl::NotFoundError("No sink file");  // First cleanup executes
+//       absl::Status result = absl::NotFoundError("No sink file");
+//       return result;  // First cleanup executes
 //     }
 //
-//     // C++11 style cleanup using the factory function
-//     auto sink_closer = absl::MakeCleanup([sink_file] { fclose(sink_file); });
+//     // Ensure the sink file is closed on scope exit...
+//     absl::Cleanup sink_closer = [sink_file] { std::fclose(sink_file); };
 //
-//     Data data;
-//     while (ReadData(source_file, &data)) {
+//     example::Data data;
+//     while (example::ReadData(src_file, &data)) {
 //       if (!data.IsGood()) {
 //         absl::Status result = absl::FailedPreconditionError("Read bad data");
 //         return result;  // Both cleanups execute
 //       }
-//       SaveData(sink_file, &data);
+//       example::SaveData(sink_file, &data);
 //     }
 //
 //     return absl::OkStatus();  // Both cleanups execute
 //   }
 // ```
 //
-// Methods:
+// Usage:
 //
-// `std::move(cleanup).Cancel()` will prevent the callback from executing.
+// Refraining from calling any methods will...
+// - Result in the callback being called when the `absl::Cleanup` goes out of
+//   scope.
+// - Leverage compiler constant folding to remove the runtime branch that checks
+//   whether the callback is engaged. If you do not touch the cleanup object
+//   after initialization, it should provide performance on par with a control
+//   flow construct such as the `defer` found in Zig. Please file a bug if you
+//   observe otherwise.
 //
-// `std::move(cleanup).Invoke()` will execute the callback early, before
-// destruction, and prevent the callback from executing in the destructor.
+// Calls to `std::move(cleanup).Cancel();` will...
+// - Destroy the callback immediately.
+// - Disengage the callback, meaning that when the `absl::Cleanup` goes out of
+//   scope, the callback will not be called.
 //
-// Usage:
+// Calls to `std::move(cleanup).Invoke();` will...
+// - Invoke the callback immediately.
+// - Destroy the callback as soon as the immediate invocation completes.
+// - Disengage the callback, meaning that when the `absl::Cleanup` goes out of
+//   scope, the callback will not be called.
+//
+// Note:
 //
 // `absl::Cleanup` is not an interface type. It is only intended to be used
-// within the body of a function. It is not a value type and instead models a
-// control flow construct. Check out `defer` in Golang for something similar.
+// within the body of a function. Please refrain from using it as a continuation
+// passing mechanism.
+//
+// The use of `std::move(cleanup).MyMethod()` as ceremony to invoke the methods
+// is intended to leverage existing best-effort tools that detect normal
+// use-after-move. The implementation is not equipped to handle multiple method
+// calls and thus has the same semantics the tools already diagnose.
 
 #ifndef ABSL_CLEANUP_CLEANUP_H_
 #define ABSL_CLEANUP_CLEANUP_H_
@@ -90,25 +113,41 @@ class [[nodiscard]] Cleanup final {
                 "Callbacks that return values are not supported.");
 
  public:
-  Cleanup(Callback callback) : storage_(std::move(callback)) {}  // NOLINT
-
-  Cleanup(Cleanup&& other) = default;
+  // NOLINTBEGIN(google-explicit-constructor)
+  Cleanup(Callback callback) {  // NOLINT(runtime/explicit)
+    storage_.EmplaceCallback(std::move(callback));
+    storage_.EngageCallback();
+  }
+  // NOLINTEND(google-explicit-constructor)
+
+  Cleanup(Cleanup&& other) {
+    ABSL_HARDENING_ASSERT(other.storage_.IsCallbackEngaged());
+    cleanup_internal::Defer last_step = [&] {
+      other.storage_.DestroyCallback();
+    };
+    other.storage_.DisengageCallback();
+    storage_.EmplaceCallback(std::move(other.storage_.GetCallback()));
+    storage_.EngageCallback();
+  }
 
   void Cancel() && {
     ABSL_HARDENING_ASSERT(storage_.IsCallbackEngaged());
-    storage_.DestroyCallback();
+    cleanup_internal::Defer last_step = [&] { storage_.DestroyCallback(); };
+    storage_.DisengageCallback();
   }
 
   void Invoke() && {
     ABSL_HARDENING_ASSERT(storage_.IsCallbackEngaged());
+    cleanup_internal::Defer last_step = [&] { storage_.DestroyCallback(); };
+    storage_.DisengageCallback();
     storage_.InvokeCallback();
-    storage_.DestroyCallback();
   }
 
   ~Cleanup() {
     if (storage_.IsCallbackEngaged()) {
+      cleanup_internal::Defer last_step = [&] { storage_.DestroyCallback(); };
+      storage_.DisengageCallback();
       storage_.InvokeCallback();
-      storage_.DestroyCallback();
     }
   }
 
@@ -118,24 +157,11 @@ class [[nodiscard]] Cleanup final {
 
 // `absl::Cleanup c = /* callback */;`
 //
-// C++17 type deduction API for creating an instance of `absl::Cleanup`
+// Explicit deduction guides signal to tooling that this type actively and
+// intentionally supports Class Template Argument Deduction.
 template <typename Callback>
 Cleanup(Callback callback) -> Cleanup<cleanup_internal::Tag, Callback>;
 
-// `auto c = absl::MakeCleanup(/* callback */);`
-//
-// C++11 type deduction API for creating an instance of `absl::Cleanup`
-template <typename... Args, typename Callback>
-absl::Cleanup<cleanup_internal::Tag, Callback> MakeCleanup(Callback callback) {
-  static_assert(cleanup_internal::WasDeduced<cleanup_internal::Tag, Args...>(),
-                "Explicit template parameters are not supported.");
-
-  static_assert(cleanup_internal::ReturnsVoid<Callback>(),
-                "Callbacks that return values are not supported.");
-
-  return {std::move(callback)};
-}
-
 ABSL_NAMESPACE_END
 }  // namespace absl