odd code in dbusbind.c

odd code in dbusbind.c

[Tom Tromey]

While looking through dbusbind.c, I found a few oddities.

In Fdbus_register_signal:

  FUNCTIONP (handler);

In Fdbus_unregister_signal:

  /* Check parameter.  */
  CONSP (object) && (!NILP (XCAR (object))) && CONSP (XCDR (object));

I think each of these statements has no effect.  Presumably the author
meant to use one of the CHECK_* macros instead.


Also, I noticed that the XD_ macros, e.g., XD_ERROR, are braced
statements.  IMO it is idiomatic, and somewhat safer, to wrap things
like this in "do ... while (0)".  That way they can be safely used in
'if's (right now the code handles this by omitting ";"s where they
would interfere -- but this is subtle and easy to mess up).

As it is, the definition of XD_DEBUG_MESSAGE introduces some ambiguous
'else's in xd_retrieve_arg.  I think this is an actual bug.


xd_read_message declares a return type of Lisp_Object but never
returns a value.  Perhaps the return value is never used -- but IMO it
would be less weird to simply always return Qnil.


These little oddities can also be found by compiling with -Wall.  That
exposes other things, too... is there a particular reason not to be
"-Wall clean"?

Tom

Re: odd code in dbusbind.c

[Michael Albinus]

Tom Tromey <tromey@redhat.com> writes:

Hi Tom,

> While looking through dbusbind.c, I found a few oddities.

Thanks a lot for your review. All mentioned points shall be fixed now.

> These little oddities can also be found by compiling with -Wall.  That
> exposes other things, too... is there a particular reason not to be
> "-Wall clean"?

No idea why it isn't the default. I've added this to MYCPPFLAGS, so that
I will see such things earlier.

> Tom

Best regards, Michael.

Re: odd code in dbusbind.c

[Tom Tromey]

>>>>> "Michael" == Michael Albinus <michael.albinus@gmx.de> writes:

Michael> Thanks a lot for your review. All mentioned points shall be
Michael> fixed now.

Thanks!

Here's one more I ran into -- I was copying parts of dbusbind.c for my
status icon patch, and this looked odd.  Other places in Emacs seem to
use non-static input_event structs this way.  kbd_buffer_store_event
could be clearer about whether the event is copied or the pointer is
stored, though...

Tom

Index: dbusbind.c
===================================================================
RCS file: /sources/emacs/emacs/src/dbusbind.c,v
retrieving revision 1.14
diff -u -c -r1.14 dbusbind.c
cvs diff: conflicting specifications of output style
*** dbusbind.c	30 Dec 2007 15:55:29 -0000	1.14
--- dbusbind.c	30 Dec 2007 21:32:56 -0000
***************
*** 962,968 ****
  {
    Lisp_Object args, key, value;
    struct gcpro gcpro1;
!   static struct input_event event;
    DBusConnection *connection;
    DBusMessage *dmessage;
    DBusMessageIter iter;
--- 962,968 ----
  {
    Lisp_Object args, key, value;
    struct gcpro gcpro1;
!   struct input_event event;
    DBusConnection *connection;
    DBusMessage *dmessage;
    DBusMessageIter iter;

Re: odd code in dbusbind.c

[Michael Albinus]

Tom Tromey <tromey@redhat.com> writes:

> Here's one more I ran into -- I was copying parts of dbusbind.c for my
> status icon patch, and this looked odd.  Other places in Emacs seem to
> use non-static input_event structs this way.  kbd_buffer_store_event
> could be clearer about whether the event is copied or the pointer is
> stored, though...

Thanks, I've applied your patch.

> Tom

Best regards, Michael.