Re: [g-a-devel]patch to make test-simple work again ...

[Bill Haneman <bill haneman sun com>]

Hi Bill,

Hi Michael: my comments prefaced by "BH".

BH: Firstly, thanks for the patch...

	Firstly; I'm surprised to see 'case' being used as a function - mostly
people like to pretend 'return' is a function: "return (3);" or
whatever, seldom have I seen "case (MY_ENUM_VALUE) :" :-) I
re-architected all the code that used that anyway.

BH: legal however, as you see.  was having an odd moment I guess ;-).

	We now track the window manager event listeners since this just dropped
out of the code - removing a number of FIXMEs - I hope it is still
obvious that we don't actually do anything with these listeners yet.

	I'm still rather concerned about registry.c (parse_event_type) this
method is called per event and it does rather a number of curious things
- particularly lots of malloc / free thrash. Also,

What is this in aid of:

  etype->event_name = g_strndup (event_name, 255);

	Why have a built in arbitrary limit ? and do a slower dup ? seems
strange to me.

BH: I'm used to doing strncpy, etc. because of stack overflow
exploits... it doesn't look to me as if g_strdup guards against failure
of g_new0 to allocate a big enough buffer.  Mebbe I am wrong here, and
just momentarily paranoid...

	Also why do we do:

          etype->major = g_quark_from_string (split_string[1]);
	  etype->minor = etype->major;
	  etype->detail = etype->major;

	Should these not be '0' so the process is at least reversible
mapping to / from the etype structure ?

BH: reversibility would be intellectually satisfying but I don't know
that it's necessary here.  The first part of the string is used for
other stuff but is always the same by the time the string parse func is
called.

BH: I'll explain this func in more detail some time...

	The purpose of this function is opaque to me, and it's
efficiency is really not good - what are we trying to achieve here ?
given that we do this per event emitted it might be worth making it
fast. Can you explain what we're trying to achieve here Bill, and I'll
write you an ultra lean, fast unrolled event type parser / quarker.

BH: Personally I think we should wait with this.  The struct is not in
fact
arbitrary ;-), and there are more important fixes to make at the moment;
optimizations here can surely wait, though I certainly don't have
anything against fast code.  

	On a different track I'm seeing lots of re-enterancy hazards around the
place [ beyond the evil list walking ones I fixed ]:

	It is important to do list tracking operations before poking at the
contents of the list in a way that can cause re-enterancy; ie. we need
to do:

	... = g_list_delete_link (..., obj's element);
	bonobo_object_release_unref (obj, NULL);

	Not:

!	bonobo_object_release_unref (obj, NULL);
!	... = g_list_delete_link (..., obj's element);

BH: How can deleting a link cause the bonobo object to be touched? 
Seems to
BH: me that this can only be a problem in a multi-threaded environment.

	I also removed the double construct referencing issue in accessible.c -
but this seemed to have no nasty side effects - which is nice.

BH: Right, as I mentioned earlier that was just a typing goof.

	So; I await your comments :-)

	Regards,

		Michael.

-static long _get_unique_id();

BH: I still maintain that it's better to make
BH: the unique id assignment programmatic, in case someday we have
multiple
BH: copies of this data segment (OK, edge case that is in the future,
but still...)

-  Accessibility_Application__set_id (application, _get_unique_id(),
ev);
+  Accessibility_Application__set_id (application, ++id, ev);
 
+#ifdef USE_A_HASH_IN_FUTURE
 static gint
 compare_corba_objects (gconstpointer p1, gconstpointer p2)
 {
@@ -139,6 +216,7 @@ compare_corba_objects (gconstpointer p1,
   retval = !CORBA_Object_is_equivalent ((CORBA_Object) p1,
(CORBA_Object) p2, &ev);
   return retval;  
 }
+#endif

+#ifdef USE_A_HASH_IN_FUTURE
+
 static gint
 compare_listener_quarks (gconstpointer p1, gconstpointer p2)
 {
-  return (!((SpiListenerStruct *)p2)->event_type_quark ==
((SpiListenerStruct *)p1)->event_type_quark);
+	return (((SpiListenerStruct *)p2)->event_type_quark !=
+		((SpiListenerStruct *)p1)->event_type_quark);
 }

 static gint
@@ -185,15 +265,16 @@ compare_listener_corbaref (gconstpointer
   return compare_corba_objects (((SpiListenerStruct *)p2)->listener,
                                 ((SpiListenerStruct *)p1)->listener);
 }
+#endif
 

??? what's with the guards here?  Looks like these methods is still
called...  couldn't we just make this a TODO, I don't think the
guards help readability at all. ;-)
 
BH: Otherwise the patch looks good, though there was a lot of
interleaving
in the diff making it really hard to read...

BH: Again, thanks for putting this one together...

:-)

-Bill