Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Pedro Alves <palves@redhat.com>]

On 01/17/2019 09:39 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> See patch below.  Since cleanup_function turned into scope_exit,
> Pedro> this new class became forward_scope_exit, because it's a "forwarding"
> Pedro> version of scope_exit?  I'm really not that sure about that name...
> Pedro> wrap_scope_exit came to mind too.  Or maybe call it cleanup_function?
> 
> The name seems reasonable enough to me.
> 
> Pedro> Ideally we'd find a way to implement SCOPE_FAILURE/SCOPE_SUCCESS
> Pedro> in C++11 instead...
> 
> Maybe we should start a C++17 meta-bug to collect future changes.
> 
> Pedro> I think this is the first time I had found a use for a function-try-block,
> Pedro> see scope_exit ctor...
> 
> TIL!
> 
> Pedro> Here is patch with everything together.  WDYT?
> 
> I think it all looks good.  One nit and one question...
> 
> Pedro> +/* A forward_scope_exit is like scope_exit, but instead of giving it a
> Pedro> +   callable, you instead specialize it for a given cleanup function,
> Pedro> +   and the generated class automatically has a constructor with the
> 
> I think it would make sense to explain the origin of the "forward" name
> somewhere in this comment.

I added this locally:

diff --git c/gdb/common/forward-scope-exit.h w/gdb/common/forward-scope-exit.h
index 6b51e296b6..5ce230f0dc 100644
--- c/gdb/common/forward-scope-exit.h
+++ w/gdb/common/forward-scope-exit.h
@@ -64,7 +64,11 @@
    callable template type when you create the gdb::optional:
 
      gdb:optional<scope_exit<what goes here?>>
-*/
+
+   The "forward" naming fits both purposes shown above -- the class
+   "forwards" ctor arguments to the wrapped cleanup function at scope
+   exit time, and can also be used to "forward declare"
+   scope_exit-like objects.  */
 
 namespace detail
 {



> 
> Pedro> +#define SCOPE_EXIT \
> Pedro> +  auto CONCAT(scope_exit_, __LINE__) = ::detail::scope_exit_lhs () + [&] ()
> [...]
> Pedro> +  auto invalidator
> Pedro> +    = make_scope_exit ([&] { this->invalidate (regnum); });
>  
> In the current spots it doesn't matter, but I tend to think it's better
> to capture by value rather than by reference.  


Capturing by value would mean that you wouldn't be able to refer to
objects of non-copyable types, though.  E.g., std::unique_ptr.  Unless
we took the effort to capture pointers to such objects, of course.
I don't think the result would be as nice.  The nice thing about capturing
by reference is that it always Just Works with no syntax overhead.


> The local being captured
> might well be reused in the function.
> 

Indeed, that's a valid concern.  How about documenting it, something like:

 -/* Register a block of code to run on scope exit.  */
 +/* Register a block of code to run on scope exit.  Note that the local
 +   context is captured by reference, which means you should be careful
 +   to avoid the case of locals being captured being inadvertently
 +   reused/changed in the function before the scope exit runs.  */
 
 #define SCOPE_EXIT \

?

Maybe there's a way to simplify that sentence.

> On the other hand, this would be a difference from the spec.

Yeah.  The SCOPE_EXIT macro itself isn't part of the p0052r5 proposal AFAICT.

It's part of Alexandruscu's original SCOPE_EXIT idea though, shown on
slide 20/46 (pdf page 14) of:
 http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf

IMHO diverging would cause people familiar with the idea confused.

I'm working on merging my changes to each appropriate patch of
your series.

Thanks,
Pedro Alves