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

  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:




> 
> 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
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

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

Yeah, good point.

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

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

Pedro> ?

Pedro> Maybe there's a way to simplify that sentence.

How about:

  ... be careful to avoid inadvertently changing a captured local's
  value before the scope exit runs.

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

Thanks!  I think there's one or two more cleanups that can be converted
this way, after this goes in.

Tom