I must admit that I don't see any point in an insertion operation that does not replace the value. And returning pointers to destroyed values doesn't seem like a good idea to me, either.

accumulating unique items is a common operation and it is more useful to phrase it as

insert_extended (... dont_override)

rather than doing a dual lookup in the non-existent case

if (NULL == lookup ()
   insert ()

The case of returning pointers to destroyed values is indeed a poor api choice and I would not expect it to be used when destroy functions are supplied.  However, it seemed preferable to have a single api addition rather than multiple methods.   It is useful to do an insertion that returns the old values if there are no destruction functions.

Created attachment 180961 [details] [review]
an alternative patch

Here is my variation on this topic. For good measure, I've thrown in the corresponding hash table api as well.

/**
 * g_tree_probe:
 * @tree: a #GTree
 * @key: the key to insert or find
 * @found_key: (allow-none): return location for the key in the tree,
 *     or %NULL
 * @found_value: (allow-none): return location for the value in the tree,
 *     or %NULL
 *
 * Looks up @key in @tree.
 *
 * If @key is present in @tree, @found_key and @found_value are
 * set to point to the locations of the found key-value pair
 * matching @key.
 *
 * If @key is not yet present in @tree, it is inserted with a value
 * of %NULL and @found_key and @found_value are set to point to the
 * locations for the newly inserted key-value pair.
 *
 * The returned @found_key and @found_value can be used to modify the
 * in-tree key-value pair, but care must be taken to preserve the
 * ordering of the key wrt. to other items that are present in the
 * tree.
 *
 * Returns: %TRUE if and existing item matching @key was found in @tree
 *
 * Since: 2.30
 */
gboolean g_tree_probe (GTree     *tree,
                       gpointer   key,
                       gpointer **found_key,
                       gpointer **found_value)


Those out parameters are perhaps a bit ugly. An alternative would be to define
a sruct 
GPair {
  gpointer key;
  gpointer value;
}

and change the signature to

GPair *g_tree_probe (GTree *tree, gpointer key);
GPair *g_hash_table_probe (GHashTable *hash, gpointer key);

That looks a bit more elegant, at the cost of an extra struct type. To check if the pair was already in the tree, you can then check if pair->key == key.

Would this kind of API work for you ?

I guess a more principled api additions would be tree iters, along the lines of

void     g_tree_iter_init      (GTreeIter *iter,
                                GTree     *tree);
gboolean g_tree_iter_previous  (GTreeIter *iter,
                                gpointer  *key,
                                gpointer  *value);
gboolean g_tree_iter_next      (GTreeIter *iter,
                                gpointer  *key,
                                gpointer  *value);
void     g_tree_iter_remove    (GTreeIter *iter);
void     g_tree_iter_steal     (GTreeIter *iter);
gint     g_tree_iter_find      (GTreeIter *iter,
                                gpointer   key,
                                gpointer  *found_key,
                                gpointer  *found_value);
void     g_tree_iter_insert    (GTreeIter *iter,
                                gpointer   key,
                                gpointer   value);

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/388.