Re: GHashTable improvements

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj gtk org> writes:

> On 16 Mar 2001, Owen Taylor wrote:
> 
> > I was looking back at Sven's patches in #50796 to see if
> > they were ready to commit. The patch adds:
> > 
> > =======
> > GHashTable* g_hash_table_new_full          (GHashFunc       hash_func,
> > 					    GEqualFunc      key_equal_func,
> > 					    GDestroyNotify  key_destroy_func,
> > 					    GDestroyNotify  value_destroy_func);
> > void        g_hash_table_destroy_no_notify (GHashTable     *hash_table);
> > void        g_hash_table_replace           (GHashTable     *hash_table,
> > 					    gpointer        key,
> > 					    gpointer        value);
> > gboolean    g_hash_table_remove_no_notify  (GHashTable     *hash_table,
> > 					    gconstpointer   key);
> > =======
> > 
> > The items that may be outstanding here are:
> > 
> >  - In the discussion earlier, there was some idea that g_hash_table_replace()
> >    wasn't necessary as long as g_hash_table_insert() called the destroy
> >    notify on the key function.
> 
> that would change the documented semantics of g_hash_table_insert() in a way
> that's very hard to debug. we had this discussion (on gnome-hackers iirc) during
> pre-1.2 and didn't change it back then either. 

To make it clear, what was planned was: 

 Whether or not g_hash_table_replace() was added, g_hash_table_insert() would free 
 the new key and keep the old key when writing over an old entry.

Changing g_hash_table_insert() was not contemplated. However, what 
g_hash_table_replace() is a possible addition, so the docs for g_hash_table_insert()
could say:

 "If there is a possibility that there is already an entry in the hash
  table for this key, you probably want to use g_hash_table_replace() instead.
  Otherwise, to make sure that the old key isn't used with the new value,
  you need to call g_hash_table_lookup() followed, in case of success, 
  by g_hash_table_remove() before calling g_hash_table_insert().
  
> so g_hash_table_insert() should stay as it is, i.e. not touching node->key if
> it's used to replace items, even if that justifies another API call
> g_hash_table_replace(). at least with that, we can tell people to use
> g_hash_table_replace() if they want to replace instead of insert.
> the replacement bahviour for _insert() just needs to be properly explained
> in the official docs and that's it.

I believe it already is, and certainly nobody was proposing changing it.
 
> >    The main argument for keeping g_hash_table_replace() then seems
> >    to be that you might have a case where key and value are
> >    associated:
> > 
> >     g_hash_table_insert (hash, entry->name, entry);
> >   
> >    I've done this fairly frequently in the past.
> > 
> >  - Do we need g_hash_table_foreach_remove_no_notify() since
> >    we have remove_no_notify() and destroy_no_notify()?
> 
> i would consider the dataset stealing API a fairly special case, and
> don't think it's such a great idea to add that everywhere we support
> destroy notifiers. so i personally would rather not see these _no_notify
> variants.

I think it's not all that uncommon to want to take a value from 
a hash table with a destroy notifier and "steal" it to add it
somewhere else, and without at least g_hash_table_remove_no_notify()
that simply isn't possible.

> >  - The name in GObject is not g_object_set_data_notify() as it
> >    was in GtkObject, but g_object_steal_data(). So perhaps,
> >    if this is our desired name, we should have
> >    g_hash_table_steal() g_hash_table_destroy_stealing_all (????)
> >    g_hash_table_foreach_steal()? 
> 
> it's not quite the same, will g_hash_table_remove_no_notify(), while
> returning the undestroyed value, still destroy the key?

I think the only sensible thing is for it to destroy neither the
key nor the value; I suppose we could have 

 g_hash_table_remove_extended (hash, key, free_key, free_value);

But that seems a little convuluted. I don't have strong feelings
about _no_notify(), except for a general feeling that you would
sometimes regret there absence. So, if there are people who definitely
want them (or definitely don't want them), let us know.

                                        Owen