[gamin] Re: gam_node.c cleanup

[Daniel Veillard <veillard redhat com>]

On Mon, Jun 13, 2005 at 12:36:33PM +0100, Neal H. Walfield wrote:
> Daniel,
> 
> I'd like to offer the attached patch (against CVS head) to clean up
> server/gam_node.c a bit.  The patch makes two primary changes: the
> documentation is cleaned up to be more internally consistent (e.g. use
> of definate and indefinite articles) and makes it a bit more complete;
> and it adds a number of asserts.
> 
> This latter change may be a bit more controversial, however, in my own
> experience, I've found it to be much better to catch potential errors
> rather than allow dubious usage.  The latter can almost always be
> changed to use the standard practices.  For instance, most of the
> gam_node_* functions allow you to pass a NULL pointer.  In a few
> cases, this might be reasonable but in most cases, passing a NULL
> pointer is likely an error.  Also, the gam_node_*_flag functions could
> assert that only a single bit is passed (the interface is clear that a
> single flag is set/unset/checked).  Someone who knows how the
> implementation works could do something like gam_node_set_flag(node,
> a_flag | b_flag).  However, in most cases multiple bits will be the
> result of a programming error.  Also, this code can easily be changed
> to set one flag at a time.
> 
> Are changes like this welcome?

  In general I appreciate cleanup patches very much, thanks for the
ones you sent already this is great !
  However I'm a bit uneasy with transforming casual return handling in case
of NULL parameters, sometimes it's just cumbersome to test NULL and having
the server exit for a perfectly recoverable reason can be just nasty. Such
an assert generated for example a number of "gam_server existed" bug report
it was good in that it allowed to chase the problem, it was bad in the
sense it made runtime hard for all users !
  Applying your patch (please use mail attachment rather than including in
the mail) for example makes some of the regression tests just crash:

    make[1]: Entering directory `/u/veillard/gamin/tests'
    [...]
    running test 8
    *** ../tests/result/8   2004-10-15 09:25:29.000000000 +0200
    --- result.8    2005-06-13 13:54:23.803253542 +0200
    ***************
    *** 1,12 ****
      connected to test
      mondir /tmp/test_gamin 0
      1: /tmp/test_gamin Deleted: NULL
    ! 1: /tmp/test_gamin EndExist: NULL
    ! mkdir /tmp/test_gamin
    ! 1: /tmp/test_gamin Created: NULL
    ! mkfile /tmp/test_gamin/foo
    ! 1: foo Created: NULL
    ! rmfile /tmp/test_gamin/foo
    ! 1: foo Deleted: NULL
    ! disconnected
    ! rmdir /tmp/test_gamin
    --- 1,4 ----
      connected to test
      mondir /tmp/test_gamin 0
      1: /tmp/test_gamin Deleted: NULL
    ! expect line 3: got 1 of 2 expected events
    running test 9

    make[2]: Entering directory `/u/veillard/gamin/python/tests'
    [...]
    basic6.py
    end from FAM server connection
    -- basic6.py
    Got callback: /u/veillard/gamin/python/tests/temp_dir, 2
    Error: top monitor got 1 events insteads of 3
    bigfile.py
   
I.e. adding assert without checking all calling points first for the
test trades immediate instability over long term code maintainance.
All code in server/ is internal code. It's not API code, so IMHO
stability is more important than API/code maintainance.

  Oh and documetnation updates are very very welcome ! If you revamp
some of it please use the

/**
 * functionname:
 * @arg1: ...
 * @argn: ...
 *
 * Description of the function
 *
 * Returns description of the return value if non void
 */

format if you can, so we can autogenerate documentation.

  I don't know what do to with your current patch, there are good points
but I don't feel like commiting if it breaks make tests. Until either of us
get to fix the breakage, I don't think the asserts should be commited.

  thanks,

Daniel

-- 
Daniel Veillard      | Red Hat Desktop team http://redhat.com/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/