Re: [PATCH 1/9] core: Create relations between keys

["Juan A." Suárez Romero <jasuarez igalia com>]

On Wed, 2011-02-23 at 16:14 +0000, Iago Toral wrote:
> > +  GList *key1_partners, *key1_peer;
> > +  GList *key2_partners, *key2_peer;
> > +
> > +  g_return_if_fail (GRL_IS_PLUGIN_REGISTRY (registry));
> > +  g_return_if_fail (key1);
> > +  g_return_if_fail (key2);
> > +
> > +  if (key1 == key2) {
> > +    return;
> > +  }
> > +
> > +  /* Search for keys related with each key */
> > +  key1_partners = g_hash_table_lookup 
> > (registry->priv->relation_keys, key1);
> > +  key2_partners = g_hash_table_lookup 
> > (registry->priv->relation_keys, key2);
> > +
> > +  /* Check if they are already related */
> > +  if (!key1_partners || !key2_partners || key1_partners == 
> > key2_partners) {
> > +    return;
> > +  }
> 
> 
>  Why do you exit if key1_partners *or* key2_partners are NULL?
>  Also, can that even happen given that you relate each key with
> itself?


If any of them are NULL means that those keys are not actually valid
keys registered in the system.

It's just a extra control.

> 
> > +  /* Merge both relations */
> > +  for (key1_peer = key1_partners;
> > +       key1_peer;
> > +       key1_peer = g_list_next (key1_peer)) {
> > +    /* Search key1 peer in key2 partners */
> > +    for (key2_peer = key2_partners;
> > +         key2_peer && key2_peer != key1_peer;
> > +         key2_peer = g_list_next (key2_peer));
> > +    if (!key2_peer) {
> > +      /* Didn't found; add it to key2 partners */
> > +      key2_partners = g_list_prepend (key2_partners,
> > +                                      key1_peer->data);
> > +    }
> > +  }
> 
>  In the inner loop you check key2_peer != key1_peer. Is this even 
>  possible? I mean if there is a key in key1_partners that is already
> in 
>  key2_partners shouldn't key1_partners and key2_partners be the same
> (and 
>  thus never reach these loops)?


Yes, you're right. Actually, none of the key1_partners should be in
key2_partners.

So probably merging both lists with g_list_concat would be enough.


	J.A.