amccarth added a comment.
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
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325964: [Utility] Simplify and generalize the CleanUp
helper, NFC (authored by vedantk, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
vsk added a comment.
Thanks! I'll clean up the issues you pointed out before committing.
https://reviews.llvm.org/D43662
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
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.
vsk updated this revision to Diff 135671.
vsk added a comment.
Herald added a subscriber: mgorny.
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
JDevlieghere added a comment.
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
labath added a comment.
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.
vsk added a comment.
In https://reviews.llvm.org/D43662#1016950, @labath wrote:
> In https://reviews.llvm.org/D43662#1016939, @vsk wrote:
>
> > > What do you think about a syntax like:
> > >
> > > lldb_utility::CleanUp cleanup_our(::close, our_socket); // The CleanUp
> > > constructor creates
I wouldn’t use std::bind though, just make a lambda.
On Thu, Feb 22, 2018 at 6:06 PM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added a comment.
>
> In https://reviews.llvm.org/D43662#1016939, @vsk wrote:
>
> > > What do you think about a syntax like:
> > >
> > >
labath added a comment.
In https://reviews.llvm.org/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
vsk added a comment.
> 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,
labath added a comment.
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...
https://reviews.llvm.org/D43662
___
lldb-commits mailing list
labath added a comment.
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
zturner added a comment.
Couldn't we just add a creator helper so that you can say:
auto cleanup = makeCleanupRAII([&] {
process_sp->Destroy(/*force_kill=*/false);
debugger.StopEventHandlerThread();
});
https://reviews.llvm.org/D43662
vsk created this revision.
vsk added reviewers: zturner, labath, davide, JDevlieghere.
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
15 matches
Mail list logo
