Macros that affect program flow. (was Re: temporary absence)

Neil Williams linux at codehelp.co.uk
Sun Feb 26 04:36:17 EST 2006


On Sunday 26 February 2006 3:17 am, you wrote:
> > Apart from that and the changes committed to trunk, does anyone have any
> > more code out-of-tree that should go into QOF 0.6.2?
>
> Yes.  I've got some changes related to QOF_COMMIT_EDIT_PART2. 

OK, I've got r13390 and r13391 (guid.c). PROBLEM:

The macros in qof-be-utils.h are awkward because they (all) change programme 
flow. We can't redefine QOF_BEGIN_EDIT as a call to qof_begin_edit because 
functions that use QOF_BEGIN_EDIT rely on it calling return if the instance 
is NULL or edit_level < 1. I don't like macros that work like that and I 
would like to deprecate each of these macros and force the use of the 
functions in libqof2.

We cannot redefine QOF_COMMIT_EDIT_PART2 because the last call in the macro is 
'return':
-  if ((inst)->do_free) {                                         \
-     (on_free)(inst);                                            \
-     return;                                                     \
-  }  

Your code simply returns from the qof_commit_edit_part2 function, it does NOT 
return from the function that called the original macro. To do that, existing 
code would have to check the return value of the function that is behind the 
macro. Not impossible, but not required by the current API either. That's an 
API change.

I got bitten by this problem when I first tried to use a function in place of 
QOF_BEGIN_EDIT.

IF the macro is NOT followed by any further code in the calling function, this 
would be OK but there is nothing to say that this is the case (or has to be 
the case). The current API (rightly or wrongly) allows programmes to call 
QOF_COMMIT_EDIT_PART2 and then continue processing the instance. Such code is 
assuming that if the instance was freed as part of the commit that the macro 
will have returned OUT of the function itself. A function replacing 
QOF_COMMIT_EDIT_PART2 will still free the instance but now processing 
continues into the rest of the function which is not expecting a freed 
instance to be possible. Even having a LEAVE macro that references the now 
freed instance would cause a seg fault.

Derek - I would v.v.v.much like to deprecate all the macros in qof-be-utils.h 
precisely because of these problems. They cause no end of confusion and 
hidden assumptions. I know you wanted these as macros but I think the API is 
too confusing. IMHO, the pros of the macros do not outweigh their problems. 
The functions are easier to use - particularly because they can be used in 
functions that return a value. To me, macros should not determine whether or 
not I can return a value from an internal function.

In fact, once the macros are deprecated (i.e. moved into deprecated.h and 
tagged in doxygen), qof-be-utils.h can be removed and the function 
declarations made part of qofbackend.h (where I think new users of libqof 
would expect to find them).

Chris: We need to replace the macro and have the function as an alternative, 
*not* a replacement. It matters not whether gnucash has a problem with the 
function, it matters that the API is changed by the change in programme flow 
generated within the old macro but not implementable in the function version.

> I have 
> verified that this macro is not used outside of GnuCash. 

That's not the whole story for an API issue - a macro in an API must retain 
it's arguments and behaviour, especially it's use of 'return;'.

i.e. code that uses the old macro must still function the same way. The macro 
is still part of the API (until libqof2). We need the macro replaced, for 
now. I don't like it, but that's the way it is until libqof2.

If there is agreement to deprecate all these macros, then the functions will 
replace all the macros in libqof2. Until then, libqof1 requires the old macro 
to be in place.
:-(

-- 

Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20060226/c64f744b/attachment-0001.bin


More information about the gnucash-devel mailing list