Problems with qof_begin_edit and qof_commit_edit

[David Hampton]

I have some concerns with these functions that have been committed to
the code over the last several months.  In some places they replace
similarly named macros, in others they are newly added to existing code.
My concerns are:

1) These function do not have the exact characteristics of the macros
they replace.  Specifically, a return call from the macro exits the
calling function.  A return in one of these functions only exits the
function, not the caller.  The caller then executes code that was not
executed prior to the macro->function change.  This behavior is what
caused the crashes that both Didier and I saw.  This is clearly broken
and must be fixed.

2) Many places have had a qof_begin_edit()/qof_commit_edit() pair of
function calls wrapped around existing code.  In this case the control
flow through the pre-exiting code is the same as before.  I.E. It always
occurs.  My concern is whether or not this is the proper control flow.
If qof_begin_edit() bails out because the object is already being
edited, should any other code be executed or should the calling function
exit immediately?  I believe the calling function should exit
immediately (i.e. don't modify an object that is being modified
elsewhere).

Fixing either of these problems requires a (backward compatible) change
to the current QOF API.  These two functions should return a boolean
value that indicate whether they executed successfully or bailed out.
This will provide information back to the calling function that it
originally had when a macro definition was used.  The calling function
can then exit immediately if the qof_xxx_edit function bails.  It seems
to me that all callers should test the return value to see if
qof_begin_edit bailed.  Calls to qof_commit_edit should test the return
value if they aren't the last line of the function.

Thoughts, anyone?

David