Page MenuHomePhabricator

[ADT] Add make_scope_exit().
ClosedPublic

Authored by timshen on Jul 25 2016, 8:57 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

timshen updated this revision to Diff 65467.Jul 25 2016, 8:57 PM
timshen retitled this revision from to [ADT] Add make_scope_exit()..
timshen updated this object.
timshen added a reviewer: chandlerc.
timshen added a subscriber: llvm-commits.
dblaikie added inline comments.
include/llvm/ADT/ScopeExit.h
29 ↗(On Diff #65467)

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

45 ↗(On Diff #65467)

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

unittests/ADT/ScopeExitTest.cpp
21–31 ↗(On Diff #65467)

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

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

37 ↗(On Diff #65467)

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

timshen updated this revision to Diff 67276.Aug 8 2016, 6:53 PM
timshen marked 2 inline comments as done.

Updated attribute position and removed unnecessary deleted ctor.

unittests/ADT/ScopeExitTest.cpp
21–31 ↗(On Diff #65467)

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?

37 ↗(On Diff #65467)

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

timshen updated this revision to Diff 67278.Aug 8 2016, 7:13 PM

Updated file comments and end of namespace comments.

timshen updated this revision to Diff 67386.Aug 9 2016, 12:06 PM

Updated tests.

timshen removed a subscriber: dblaikie.
dblaikie accepted this revision.Aug 9 2016, 1:35 PM
dblaikie edited edge metadata.
dblaikie added inline comments.
unittests/ADT/ScopeExitTest.cpp
22–32 ↗(On Diff #67386)

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);
This revision is now accepted and ready to land.Aug 9 2016, 1:35 PM
timshen updated this revision to Diff 67553.Aug 10 2016, 10:57 AM
timshen edited edge metadata.

Simplified the test case as suggested.

This revision was automatically updated to reflect the committed changes.

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 ↗(On Diff #67553)

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 marked an inline comment as done.Aug 10 2016, 11:17 AM
timshen added inline comments.
include/llvm/ADT/ScopeExit.h
33–34 ↗(On Diff #67553)

Done. Made it move-only in r278259.

timshen marked an inline comment as done.Aug 10 2016, 3:44 PM

Also fixed the LLVM_ATTRIBUTE_UNUSED_RESULT issue in r278298.