Re: [gamin] gam_connection.c clean up

[Daniel Veillard <veillard redhat com>]

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. The compiler may detect it's
zero and then put it on the BSS. I tend to always initialize static
variables, it's an habit, I'm not sure this should be reversed....

> 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. I wonder if there is a way to switch off g_assert()
when producing final code. The problem is then to make sure there is 
absolutely no side effects in the assertions code.

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 ? 

  Applied anyway, thanks a lot !

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/