Re: FYI: unwind_protect improved

[Jaroslav Hajek]

On Tue, Jun 23, 2009 at 3:26 PM, John W. Eaton<address@hidden> wrote:
> On 23-Jun-2009, Jaroslav Hajek wrote:
>
> | There's a generic unwind_protect::protect_var method able to handle a
> | variable of any assignable & copyable class.
> | unwind_protect::protect_mem is also provided though it will probably
> | be very seldom used (given that you can protect a NDArray or whatever
> | with protect_var).
> |
> | using string tags for unwind_protect frames is now deprecated:
> |
> |   unwind_protect::begin_frame ("tree_evaluator::visit_simple_for_command");
> | ...
> |   unwind_protect::run_frame ("tree_evaluator::visit_simple_for_command");
> |
> | In particular, if you misspelled the string in run_frame, it would
> | silently run all frames down the stack (!).
> |
> | The proper usage is now:
> |
> |   unwind_protect::frame_id_t uwp_frame = unwind_protect::begin_frame ();
> | ...
> |   unwind_protect::run_frame (uwp_frame);
> |
> | which allocates the frame info as a local variable (in fact, it's
> | simply the stacksize at the moment of query). This is also somewhat
> | more efficient, as there's no string manipulation when processing the
> | stack, neither is there need to store them in the stack.
> |
> | The old tags mechanism is still supported, but the methods are marked
> | as deprecated. They should be removed at some point.
> |
> | It is also possible to protect a single variable locally, if
> | performance is of particular concern:
> |
> | {
> |    unwind_protect::restore_var<bool> flag_protect (flag); // will
> | auto-restore flag at end of block
> |    flag = temporary value;
> |    ...
> | }
> |
> | this is more efficient because the compiler can inline everything.
>
> Thanks.  This is a big improvement.  The old design was copied from
> bash early in Octave's history.  Since bash is written in C, it
> can't take advantage of these kinds of constructor/destructor
> features, and I wasn't smart enough to see a better way.  Oh, and
> templates weren't part of C++ yet...
>

Oh, so that's where it came from... now it makes more sense. To be
honest, I don't particularly like the global unwind_stack idea - it
doesn't play nicely with user handling using try-catch and
(especially) try-finally, which I think is more C++-ish. The problem
is that when interrupt or bad_alloc occurs, the whole unwind_protect
stack is run (via hook), then an exception is raised which unwinds the
(system) call stack. If a user tries to handle something ussing
try/finally, in the finally block the unwind_protect stack will
already be processed, and the user must be aware of it - but it seems
counter-intuitive to me.
OTOH, we can't empty the stack after the exception is propagated to
the top level, because the cleanup handlers can (and do, in some
places) refer to local variables.
In a certain sense, the unwind_protect mechanism is more powerful
(allows dynamic adding of handlers), but does Octave really need it?
Wouldn't error handling based purely on try/catch/finally be
sufficient? Maybe faster, too. I think gcc has gone far with exception
handling optimization. What do you think?

FYI, I pushed in some more improvements concerning unwind_protect,
atexit, quit, Ctrl-C and the like. It seems octave_quit_exception was
not that good idea after all, as it does not propagate through
liboctave Fortran callbacks, and cleaning unwind_stack has the
problems described above. For now, I've removed it. I also
transplanted the patch to 3.2.x. octave_quit_exception was not marked
as API class, and probably not used by any C++ code, so I don't see
this as a problem.

The following three programs (also attached) will now do what you
think they should (fail in 3.2.0):

tt1.m:

function xdot = ttt (x, t)
  xdot = 1;
  unwind_protect
    if t > 0.5
      t
      quit();
    endif
  unwind_protect_cleanup
    disp ("hi");
  end_unwind_protect
endfunction

unwind_protect
  a = lsode (@ttt, 1, 0:.1:1);
unwind_protect_cleanup
  disp ("hihi");
end_unwind_protect

tt2.m:

atexit ("quit")
quit()

tt3.m:

function dumb
  fprintf (stderr, "press Ctrl-C:\n")
  unwind_protect
    sleep (10)
    quit ()
  unwind_protect_cleanup
    disp ("out!")
  end_unwind_protect
endfunction
atexit("dumb")
unwind_protect
  quit ()
unwind_protect_cleanup
  fprintf (stderr, "me too!\n")
end_unwind_protect

> I think the names "saved_var" and "saved_mem" might be better than
> "restore_var" and "restore_mem".  It usually seems better to me to
> name objects as things rather than actions.

I wanted to the word "restore" in the name to emphasize that it will
restore the value on cleanup, and "restorator" or something seemed bad
to me. I don't think saved_var quite hits the meaning, but if you
think so, feel free to change it.

> You have
>
>    private:
>
>    // No copying!
>    void operator = (const restore_mem&);
>
> in the restore_mem class (and a similar declaration in the restore_var
> class) but I don't think this actually prevents copying.  For example,
> try this program:
>
>  class foo
>  {
>  public:
>    foo (void) { }
>    ~foo (void) { }
>  private:
>    void operator = (const foo&);
>  };
>
>  int
>  main (void)
>  {
>    foo x;
>    foo y (x);
>    foo z = y;
>    return 0;
>  }
>
> I think that using
>
>    private:
>      restore_mem (const restore_mem&);
>
> is sufficient, but I usually write
>
>    private:
>      // No copying!
>      X (const X&);
>      X& operator = (const X&);
>
> to prevent copying in a class X.
>
> jwe
>

OK, I added the copy constructors.

cheers

-- 
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

tt1.m
Description: Text Data

tt2.m
Description: Text Data

tt3.m
Description: Text Data