This is an archive of the discontinued LLVM Phabricator instance.

[Utility] Simplify and generalize the CleanUp helper, NFC
ClosedPublic

Authored by vsk on Feb 22 2018, 5:29 PM.

Details

Summary

Removing the template arguments and most of the mutating methods from
CleanUp makes it easier to understand and reuse.

In its present state, CleanUp would be too cumbersome to adapt to cases
where multiple objects need to be released. Take for example this change
in swift-lldb:

https://github.com/apple/swift-lldb/pull/334/files#diff-6f474df750f75c8ba675f2a8408a5629R219

This change is simple to express with the new CleanUp, but not so simple
with the old version.

Diff Detail

Event Timeline

vsk created this revision.Feb 22 2018, 5:29 PM

Couldn't we just add a creator helper so that you can say:

auto cleanup = makeCleanupRAII([&] {
    process_sp->Destroy(/*force_kill=*/false);
    debugger.StopEventHandlerThread();
  });

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

This would be a generalization of your syntax, so you could still use a lambda when needed, but you could avoid the lambda-capture dance for the simple cases.

Btw, I think we should also move the CleanUp class out of the lldb_utility and into lldb_private namespace. we have been slowly getting rid of these...

vsk added a comment.Feb 22 2018, 5:53 PM

Couldn't we just add a creator helper so that you can say:

auto cleanup = makeCleanupRAII([&] {

  process_sp->Destroy(/*force_kill=*/false);
  debugger.StopEventHandlerThread();
});

@zturner In this version of the patch, CleanUp accepts a std::function, which gets rid of the lifetime issue with llvm::function_ref. So with this you should bee able to write "auto cleanup = CleanUp([&] { ... })". Does that work for you?

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

@labath I find the current version of the code more readable, since the behavior of the cleanup is apparent in less time. And I'd prefer to keep the CleanUp utility very small, without any scope for expansion. I don't think the ergonomics of the new syntax outweigh the advantages of a simpler approach.

we should also move the CleanUp class out of the lldb_utility and into lldb_private namespace

Sounds good, I'll update the patch once we hash out the interface.

In D43662#1016939, @vsk wrote:

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

@labath I find the current version of the code more readable, since the behavior of the cleanup is apparent in less time. And I'd prefer to keep the CleanUp utility very small, without any scope for expansion. I don't think the ergonomics of the new syntax outweigh the advantages of a simpler approach.

I don't think this would complicate anything. It should literally be a matter of replacing your constructor with:

template<typename F, typename... Args>
CleanUp(F &&f, Args &&..args)
    : PerformCleanup(true), Clean(std::bind(std::forward<F>(f), std::forward<Args>(args)...) {}

Although I can see how you may find the other usage syntax more understandable, so I am not adamant about this...

vsk added a comment.Feb 22 2018, 6:32 PM
In D43662#1016939, @vsk wrote:

What do you think about a syntax like:

lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp constructor creates the std::function internally

@labath I find the current version of the code more readable, since the behavior of the cleanup is apparent in less time. And I'd prefer to keep the CleanUp utility very small, without any scope for expansion. I don't think the ergonomics of the new syntax outweigh the advantages of a simpler approach.

I don't think this would complicate anything. It should literally be a matter of replacing your constructor with:

template<typename F, typename... Args>
CleanUp(F &&f, Args &&..args)
    : PerformCleanup(true), Clean(std::bind(std::forward<F>(f), std::forward<Args>(args)...) {}

Although I can see how you may find the other usage syntax more understandable, so I am not adamant about this...

Thanks for sharing this (I haven't reached for parameter packs before :). I'll still maintain that having the lambda written-out explicitly requires less effort from prospective readers, but am happy to be outvoted on this one. Pavel, others, wdyt?

While we wait for the verdict on the template stuff, it occurred to me that you don't need the PerformCleanup bool member as std::function has an "empty" state. So the destructor can just do if(Cleanup) Cleanup() and the disable function can reset the cleanup object. (If you want to make sure the object is always created with a non-empty std::function initially, you can add an assert in the constructor).

I agree with Pavel, I prefer having a slightly more complex constructor if we can make the call sites simpler for the simple common cases, especially since we don't lose any generality by doing so. It's also not an uncommon pattern in C++, e.g. std::async does something very similar.

Also +1 on just calling operator bool on the function itself.

vsk updated this revision to Diff 135671.Feb 23 2018, 11:27 AM

Thanks for the feedback.

I've added support for forwarding arguments to the cleanup function though the constructor. It uses std::bind instead of a lambda because I couldn't figure out a simpler way to support reference arguments. (Btw, llvm::ThreadPool does the same thing.)

I've also added a unit test. PTAL.

labath accepted this revision.Feb 23 2018, 2:03 PM

This looks great, thanks. I've been wanting to do something about the CleanUp class for a long time. I have just a couple of nits, but no need to go through review again.

unittests/Utility/CleanUpTest.cpp
1

s/test/Test/

13

Looks like this is left-over from testing.

This revision is now accepted and ready to land.Feb 23 2018, 2:03 PM
vsk added a comment.Feb 23 2018, 2:03 PM

Thanks! I'll clean up the issues you pointed out before committing.

This revision was automatically updated to reflect the committed changes.

Nit: As a single word, "cleanup" is a noun (or an adjective). As two words, "clean up" is a verb. Given that, I'd expect to see classes and objects with names like Cleanup or cleanup and functions to contain CleanUp. When I see an identifier like CleanUpTest, I expect it's a function that cleans up after a test, not a test of a class called CleanUp.