Created attachment 213026 [details] [review]
Adds g_list_copy_deep()

Created attachment 213027 [details] [review]
Adds g_slist_copy_deep()

ping

Seems like you could generalize this a bit as g_list_map().

But...what's the use case?  What code pattern requires you to deep copy lists?

suppose you have a list of gobjects and want a full duplicated list. so, you can pass g_object_ref.

at the time we had a little irc chat and derst liked the idea.

(In reply to comment #5)
> suppose you have a list of gobjects and want a full duplicated list.

The concrete question is - what projects would use this code, and where would they use it?  Presumably you have one?

(In reply to comment #6)
> (In reply to comment #5)
> > suppose you have a list of gobjects and want a full duplicated list.
> 
> The concrete question is - what projects would use this code, and where would
> they use it?  

Also, just try running "git grep g_list_copy" in glib.  Looks like there's at least one potential user in gio/gunixvolumemonitor.c

(In reply to comment #6)
> (In reply to comment #5)
> > suppose you have a list of gobjects and want a full duplicated list.
> 
> The concrete question is - what projects would use this code, and where would
> they use it?  Presumably you have one?

I'm using it in a proprietary software in my company. that's why I've cooked that patch. It seems useful for other people, and nice to have it in upstream for everyone consume.

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > suppose you have a list of gobjects and want a full duplicated list.
> > 
> > The concrete question is - what projects would use this code, and where would
> > they use it?  Presumably you have one?
> 
> I'm using it in a proprietary software in my company. 

OK, but you could at least explain *why* you need to copy+ref a list.  Often for example if you don't need to mutate the list, passing around a GPtrArray that you just ref/unref is *significantly* more efficient.

> that's why I've cooked
> that patch. It seems useful for other people, and nice to have it in upstream
> for everyone consume.

But you need to at least add tests.  Finding a "real-world" user inside GLib or GNOME (like the gio/gunixvolumemonitor.c case I pointed out) also helps a lot in arguing for new API.

> OK, but you could at least explain *why* you need to copy+ref a list.  Often
> for example if you don't need to mutate the list, passing around a GPtrArray
> that you just ref/unref is *significantly* more efficient.

In my case, it's a multithread program, so, between the time I do the copy and the time I'll process the list, other thread might touch the list and its elements, doing an unref for instance.

> 
> > that's why I've cooked
> > that patch. It seems useful for other people, and nice to have it in upstream
> > for everyone consume.
> 
> But you need to at least add tests.  Finding a "real-world" user inside GLib or
> GNOME (like the gio/gunixvolumemonitor.c case I pointed out) also helps a lot
> in arguing for new API.

I've found some users like

clutter (clutter-master-clock.c):
  timelines = g_slist_copy (master_clock->timelines);
  g_slist_foreach (timelines, (GFunc) g_object_ref, NULL);

telepathy (connection-contact-info.c): tp_contact_info_spec_list_copy ()

gnome-shell (shell-polkit-authentication-agent.c)
  request->identities = g_list_copy (identities);
  g_list_foreach (request->identities, (GFunc) g_object_ref, NULL);

I stopped grepping...

we use g_list_foreach(list, g_object_ref, NULL) in a couple of places where we iterate over a list because we need to guarantee that the list elements survive the end of the iteration - each iteration could change the list, or release a reference on the elements.

having a g_list_copy_deep() (I'm partial to g_list_copy_full() tho) would essentially remove one line, but after all we did that for g_list_free_full().

Review of attachment 213026 [details] [review]:

::: glib/glist.c
@@ +610,3 @@
+ *
+ * In contrast with g_list_copy(), this function does copy the actual data for
+ * each element in the list. It uses @func as a function that actually copies

How about:

"In contrast with g_list_copy(), this function uses @func to make a copy of each list element, in addition to copying the list container itself."

@@ +636,3 @@
+  GList *new_list = NULL;
+
+  if (list)

Might allow passing NULL for func too, then you could change g_list_copy() to just call g_list_copy_deep (list, NULL, NULL) and avoid the code duplication.

Review of attachment 213027 [details] [review]:

Can you fold these two patches together?  And add a link to the bug in the text description.  See:

https://live.gnome.org/GnomeLove/SubmittingPatches

Created attachment 216937 [details] [review]
patch v2

New patch looks fine except it's still lacking any test coverage.  Add one to glib/tests/list.c please.

Created attachment 216955 [details] [review]
patch v3

Review of attachment 216955 [details] [review]:

Some minor things, feel free to commit after adjustments.  Thanks for the patch!

::: glib/glist.h
@@ +82,3 @@
 					 GList            *link_) G_GNUC_WARN_UNUSED_RESULT;
 GList*   g_list_reverse                 (GList            *list) G_GNUC_WARN_UNUSED_RESULT;
 GList*   g_list_copy                    (GList            *list) G_GNUC_WARN_UNUSED_RESULT;

One thing I forgot before; this needs to be annotated GLIB_AVAILABLE_IN_2_34

::: glib/gslist.h
@@ +81,3 @@
 					  GSList           *link_) G_GNUC_WARN_UNUSED_RESULT;
 GSList*  g_slist_reverse                 (GSList           *list) G_GNUC_WARN_UNUSED_RESULT;
 GSList*  g_slist_copy                    (GSList           *list) G_GNUC_WARN_UNUSED_RESULT;

GLIB_AVAILABLE_IN_2_34

::: glib/tests/list.c
@@ +410,3 @@
+  for (u = l, v = l2; u && v; u = u->next, v = v->next)
+    {
+      g_assert (GPOINTER_TO_INT (u->data) * 2 == GPOINTER_TO_INT (v->data));

This is totally minor, but I'd use g_assert_cmpint() here, since it gives you better error messages.

thanks for your help, patch pushed.