Re: [gamin] gam_connection.c clean up

[Daniel Veillard <veillard redhat com>]

On Tue, Jun 14, 2005 at 02:41:06PM +0100, Neal H. Walfield wrote:
> At Tue, 14 Jun 2005 08:30:06 -0400,
> Daniel Veillard wrote:
> >   I have got my share of debugging on embedded systems where this
> > wasn't the case and always initialize now.
> 
> Interesting.  ELF says that variables in the BSS are initialized to
> zero.  Out of curiosity, what platforms?

  educational embedded systems, not ELF :-)

> > I tend to always initialize static
> > variables, it's an habit, I'm not sure this should be reversed....
> 
> It is just a bit of bloat so not a big deal.

  yeah not a big deal, just an explanation of my current practice.

> > > I renamed req_read to
> > > request_len which is a bit more intuitive.  I made request into an
> > > anonymous union as casting a GAMPacket to a void * is invalid when
> > > strict variable aliasing is enabled (which is highly desirable).  I
> > > removed gam_close_a_connection which was pretty useless.  I optimized
> > > the memmove in gam_connection_data a bit.
> > 
> >   I'm starting to get scared by the raw amount of g_assert(), the code
> > still seems to work, the regression tests still pass but my experience 
> > with gamin is that the API is often used in crazy ways and it's hard to
> > reproduce with tests.
> 
> My goal was to reduce the amount of craziness in the use of the API.

  Which is an excellent goal !

> > I wonder if there is a way to switch off g_assert()
> 
> Define G_DISABLE_ASSERT .  But I would recommend not to do this by
> default.  (That is, let distributions worry about it.)

  I am a distribution, actually 2 of them.

> > when producing final code. The problem is then to make sure there is 
> > absolutely no side effects in the assertions code.
> 
> I have been very careful to make sure that there are no side effects.

  Okay, I trust you :-)

> > I'm worried by
> > 
> > @@ -316,15 +298,10 @@
> >              events = GAMIN_EVENT_CHANGED | GAMIN_EVENT_CREATED |
> >                  GAMIN_EVENT_DELETED | GAMIN_EVENT_MOVED |
> >                  GAMIN_EVENT_EXISTS;
> > -            if (type == GAM_REQ_DIR) {
> > -                is_dir = TRUE;
> > -               events |= GAMIN_EVENT_ENDEXISTS;
> > -            } else {
> > -                is_dir = FALSE;
> > -            }
> > -            sub = gam_subscription_new(&req->path[0], events, req->seq,
> > -                                      is_dir, options);
> > 
> > +           is_dir = (type == GAM_REQ_DIR);
> > +            sub = gam_subscription_new(req->path, events, req->seq,
> > +                                      is_dir, options);
> >              gam_subscription_set_listener(sub, conn->listener);
> >              gam_add_subscription(sub);
> >              break;
> > 
> >   it does introduce a change in the list of accepted events from the
> > connection if it is a directory watch request. What is the reason for
> > that change ? 
> 
> gam_subscription_new adds GAMIN_EVENT_EXISTS and GAMIN_EVENT_ENDEXISTS
> implicitly.  I've added this to the documentation for my
> gam_subscription.c clean up.

  okay, good ! 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/