Re: [g-a-devel]at-spi event crack smoking ...

[Michael Meeks <michael ximian com>]

Hi Bill,

On Wed, 2002-08-14 at 12:46, Bill Haneman wrote:
> I appreciate constructive comments about at-spi, but this sort of public
> commentary is unnecessary and inappropriate.

	That I was appalled ? I was, I didn't mean to say it so bluntly in
public, but it's true. As for code review, it should all be in public -
I'll do a little later in this mail, it might be painful.

> Marc and I are the maintainers of this module.  As such it's perfectly
> normal for us to commit changes without public review in some
> situations

	Fine - but hopefully you're not going to be breaking the ('my') code.
Since I developed the Accessible cache as an optimization and
necessarily to make the pointer comparisons work, it would have been
nice to see changes to it.

> though I agree that public posting of patches is ideally a
> better way to go, when other considerations such as urgency and time do
> not mitigate against it.

	I understand you're under pressure, but simply reading the patch before
committing is common practice, and it's not far from there to:

	cvs diff | tee file_to_send

> I do not think that the tone of your post is warranted by the source
> issues you raise.  

	Well, since you don't think this is serious I've covered this issue in
more detail here; I noticed another set of interesting inefficiencies
added in the registry event propagation code as well that concern me,
but aren't buggy at first glance.

	So; lets examine this unwarrented concern, which let me stress, is
mostly about who is qualified to commit what to what:

> > 	This:
> > 
> > 	/*
> > 	 * must ref before doing "direct pointer" identity comparisons,
> > 	 * e.g. "accessible == a".
> > 	 * Alternatively, one can use cspi_object_equal (a, b)
> > 	 */
> > 	Accessible_ref (event->source); 
> > 		
> > 	Is just rubbish.
> 
> I beg to differ.  Direct pointer comparisons may work for ORBit2 but
> they do not work for the general case where object references may come
> from other ORBs.  

	Ok; clearly you know something I don't. Let's de-construct the
original:

> > 	 * must ref before doing "direct pointer" identity comparisons,
> > 	 * e.g. "accessible == a".

	What does that mean ? does it mean that:

	Accessible_ref (event->source);

	is somehow going to change the pointer value of event->source such that
it will now compare for equality well with other Accessible*'s pointing
at the same remote accessible? it cannot do that; there is no reference
to that pointer to allow it to change.

	Lets look at what ended up in the 'ref' code:

void
cspi_object_ref (Accessible *accessible)
{
  g_return_if_fail (accessible != NULL);

+ if (!g_hash_table_lookup (cspi_get_live_refs(), accessible->objref)){ 
+	  accessible->objref = cspi_dup_ref (accessible->objref);
+	  g_hash_table_insert (cspi_get_live_refs (),
+                              accessible->objref, accessible);
+  } else {
	  accessible->ref_count++;
+  }
}

	So, this inserts it into the live refs hash; _If_ it wasn't there
already; so in a nutshell, this means we _might_ be able to do a direct
pointer comparison, next time - if there was no instance of this
Accessible around beforehand, and if we look up the live ref table
before creating new accessibles.

	Worse, we can only compare in this one (first) listener callback, with
a reference we got from a static method called subsequently, or in that
callback; since the contemporary code[1] with this created a new
Accessible * which it leaked, per event emission, instead of looking it
up in the live ref table.

	Finally, as an API design choice - how good is it to demand people
'ref' something before being able to compare it ? do we want to add
this 4 line comment everywhere we do a comparison, along with a
potential CORBA round trip:

	ref (a)
	if (a == b) ...
	unref (a);

	?

> I beg to differ.  Direct pointer comparisons may work for ORBit2 but
> they do not work for the general case where object references may come
> from other ORBs.  

	Well, again it seems you know something I don't. You put your finger on
an issue that should be of no concern to us, related to CORBA proxying,
mapping, multiple clients per POA object and other aesoteric and un-used
(in Gnome) CORBA features. I don't believe the Java ORB gets this wrong
either, it would be too heinous.

	So; your comment said:

> > 	/*
> > 	 * must ref before doing "direct pointer" identity comparisons,
> > 	 * e.g. "accessible == a".

	we've seen that's a curious assertion.

> > 	 * Alternatively, one can use cspi_object_equal (a, b)
> > 	 */

	And now for this; so - firstly, it's nonsensical to make people use an
equality test, since we can trivially map it to pointer equality and
that is the general strategy I took when implementing it. Indeed, there
is no public API to do comparison of Accessibles beyond pointer
equality.

	Secondly, cspi_object_equal, cspi_object_hash are the methods used to
achieve the built property of pointer equality of Accessibles that
people rely on. Thus, how can they be possibly used to 'get around' this
'problem' ?

	Thirdly, and most tellingly:

static gboolean
cspi_object_equal (gconstpointer a, gconstpointer b)
{
  CORBA_Object objecta = (CORBA_Object) a;
  CORBA_Object objectb = (CORBA_Object) b;
  gboolean     retval;

  retval = CORBA_Object_is_equivalent (objecta, objectb, &ev);

  return retval;
}

	Which apart from the redundant return value, calls
CORBA_Object_is_equivalant the code for which is:

/*
 * We already ensure uniqueness of the CORBA_Object structure
 */
CORBA_boolean
CORBA_Object_is_equivalent (CORBA_Object       obj,
			    CORBA_Object       other_object,
			    CORBA_Environment *ev)
{
	return obj == other_object;
}

	This is how CORBA works; any other way is gallopingly inefficient,
that's what the spec says, and I heartily concur.

	So - in conclusion - none of these mistakes are suprising or
particularly unusual; what is - is that you feel happy adding hacks and
bodges to that ('my') code seemingly with a fairly deep level of
non-understanding going on somewhere, either on your part or mine ...
either of which is most serious.

	Regards,		

		Michael.

[1] - cvs upd -D '2002-08-11' eg.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot