Re: [gamin] gam_connection.c clean up

["Neal H. Walfield" <neal walfield org>]

At Tue, 14 Jun 2005 08:30:06 -0400,
Daniel Veillard wrote:
> 
> On Tue, Jun 14, 2005 at 12:44:43PM +0100, Neal H. Walfield wrote:
> > A clean up for gam_connection.c.
> > 
> > The patch includes the normal documentation fixes and the addition of
> > a number of g_assert()s.  There are a number of small other changes.
> > I marked gamConnList as static and don't explicitly initialize it (all
> > variables in the BSS are initialized to 0).
> 
>   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?

> The compiler may detect it's
> zero and then put it on the BSS.

It can't do this.  Idioms like `int __multiple_libcs = 0;' require
that it not happen.

> 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.

> > 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.

> 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.)

> 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.

> 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.

Thanks,
Neal