[ADT] Add make_scope_exit().

[timshen]

Summary

make_scope_exit() is described in C++ proposal p0052r2, which uses RAII to do cleanup works at scope exit.

[dblaikie]

include/llvm/ADT/ScopeExit.h

30

This is probably unnecessary - no default ctor is provided if you have any other dtor, right?

46

You could consider putting the attribute on the scope_exit type? ('spose it doesn't make much difference)

unittests/ADT/ScopeExitTest.cpp

22–32

This test seems a bit complicated - lambdas producing lambdas, a std::vector, etc.

What are the actual behaviors you're trying to test here?

38

I take it this static cast is needed for some reason? (but isn't needed for EXPECT_FALSE?)

[timshen]

Updated attribute position and removed unnecessary deleted ctor.

unittests/ADT/ScopeExitTest.cpp

22–32

Sorry, if std::vector raises surprises, it's a mistake. Changed to SmallVector.

I'd like to test the order of the scope_exit runs. I label each scope_exit with a number (x for make_scope_exit(f(x))), and when the scope exits, the label is pushed to the vector "v". Then I inspect v to make sure of the order of labels.

Do you have any suggestions to make it simpler?

38

Yes, otherwise it doesn't compile - it's something to do with ASSERT.

[timshen]

Updated file comments and end of namespace comments.

[timshen]

Updated tests.

[dblaikie]

unittests/ADT/ScopeExitTest.cpp

23–33

I'd probably change this test to have a move-only type to ensure that the the scope exit device doesn't accidentally depend on copyability anywhere, syntactically, even if it doesn't execute it dynamically.

In theory you could/should have both tests (one could imagine a bug where the thing works correctly for a move-only type, but when given a move-and-copy type, it accidentally copies), but that doesn't seem to be worth worrying about for my money as you sort of have to go out of your way to make such a problem & until the implementation is more complicated such that those sort of problems are a risk, I wouldn't bother testing it.

(& as a bonus, then there's not even any reason to have any tracking of which, if any, constructors, etc, were called - there is only move)

Honestly - at that point is the "Basic" test worth keeping? I'm not sure.

Picturing just one test, something like:

struct Callable {
  bool &Called;
  Callable(bool &Called) : Called(Called) { }
  Callable(Callable &&RHS) : Called(RHS.Called) { }
  void operator()() { Called = true; }
};
bool Called = false;
{
  auto g = make_scope_exit(Callable(Called));
  EXPECT_FALSE(Called);
}
EXPECT_TRUE(Called);

[timshen]

Simplified the test case as suggested.

[dblaikie]

Maybe add some comments - one explaining that the test case is (also) testing that the device supports move-only functors. Another explaining that the device is inspired by the standard one, though is not complete - but can be made so (ie: it wasn't deliberately made not to match, just "this is all we need for now").

include/llvm/ADT/ScopeExit.h

33–34

These won't work in the MSVC versions we need to support (they don't support synthesizing move ops at all) - so you'll have to write them out.

Also, should this type be copyable? I guess if the functor is copyable?

I'd probably just make it move-only for now & you'll have to write out the move ctor explicitly.

[timshen]

include/llvm/ADT/ScopeExit.h

33–34

Done. Made it move-only in r278259.

[timshen]

Also fixed the LLVM_ATTRIBUTE_UNUSED_RESULT issue in r278298.

include/llvm/ADT/ScopeExit.h

+//===- llvm/ADT/ScopeExit.h - Execute code at scope exit --------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the make_scope_exit function, which executes user-defined
+// cleanup logic at scope exit.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ADT_SCOPE_EXIT_H
+#define LLVM_ADT_SCOPE_EXIT_H
+
+#include "llvm/Support/Compiler.h"
+
+#include <type_traits>
+#include <utility>
+
+namespace llvm {
+namespace detail {
+
+template <typename Callable> class LLVM_ATTRIBUTE_UNUSED_RESULT scope_exit {
+  Callable ExitFunction;
+
+public:
+  template <typename Fp>
+  explicit scope_exit(Fp &&F) : ExitFunction(std::forward<Fp>(F)) {}
+
+  scope_exit(const scope_exit &) = default;
+  scope_exit(scope_exit &&) = default;
+
+  ~scope_exit() { ExitFunction(); }
+};
+
+} // end namespace detail
+
+// Keeps the callable object that is passed in, and execute it at the
+// destruction of the returned object (usually at the scope exit where the
+// returned object is kept).
+//
+// Interface is specified by p0052r2.
+template <typename Callable>
+detail::scope_exit<typename std::decay<Callable>::type>
+make_scope_exit(Callable &&F) {
+  return detail::scope_exit<typename std::decay<Callable>::type>(
+      std::forward<Callable>(F));
+}
+
+} // end namespace llvm
+
+#endif

unittests/ADT/CMakeLists.txt

set(ADTSources
   PointerEmbeddedIntTest.cpp
   PointerIntPairTest.cpp
   PointerSumTypeTest.cpp
   PointerUnionTest.cpp
   PostOrderIteratorTest.cpp
   PriorityWorklistTest.cpp
   RangeAdapterTest.cpp
   SCCIteratorTest.cpp
+  ScopeExitTest.cpp
   SequenceTest.cpp
   SetVectorTest.cpp
   SmallPtrSetTest.cpp
   SmallStringTest.cpp
   SmallVectorTest.cpp
   SparseBitVectorTest.cpp
   SparseMultiSetTest.cpp
   SparseSetTest.cpp

unittests/ADT/ScopeExitTest.cpp

+//===- llvm/unittest/ADT/ScopeExit.cpp - Scope exit unit tests --*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/ScopeExit.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(ScopeExitTest, Basic) {
+  struct Callable {
+    bool &Called;
+    Callable(bool &Called) : Called(Called) {}
+    Callable(Callable &&RHS) : Called(RHS.Called) {}
+    void operator()() { Called = true; }
+  };
+  bool Called = false;
+  {
+    auto g = make_scope_exit(Callable(Called));
+    EXPECT_FALSE(Called);
+  }
+  EXPECT_TRUE(Called);
+}
+
+} // end anonymous namespace