GNOME Bugzilla – Bug 754721
Support g_autoptr() for all libsoup object types
Last modified: 2015-09-15 14:14:03 UTC
.
Created attachment 310889 [details] [review] Support g_autoptr() for all libsoup object types This allows using e.g. g_autoptr(SoupSession) in other programs, but does not make libsoup itself use g_autoptr, or require a new enough glib to support it.
Comment on attachment 310889 [details] [review] Support g_autoptr() for all libsoup object types >+ * Copyright (C) 2015 Red Hat, Inc. No "(C)". (Yes I know most existing files have it. They're wrong.) These ought to be SOUP_AVAILABLE_IN_2_52, though it seems like it's probably not possible to do that? Maybe wrap #if SOUP_VERSION_MAX_ALLOWED >= SOUP_VERSION_2_52 ... #endif around it? Need to update docs/ for this. Probably just adding soup-autocleanups.h to IGNORE_HFILES is the right answer.
(In reply to Dan Winship from comment #2) > Comment on attachment 310889 [details] [review] [review] > Support g_autoptr() for all libsoup object types > > >+ * Copyright (C) 2015 Red Hat, Inc. > > No "(C)". (Yes I know most existing files have it. They're wrong.) Ha, and I thought it's just me who finds it weird to write both 'Copyright' and '(C)'. > These ought to be SOUP_AVAILABLE_IN_2_52, though it seems like it's probably > not possible to do that? Maybe wrap > > #if SOUP_VERSION_MAX_ALLOWED >= SOUP_VERSION_2_52 > ... > #endif > > around it? I don't know about that -- glib's and gtk's /usr/include/glib-2.0/glib/glib-autocleanups.h /usr/include/gtk-3.0/gtk/gtk-autocleanups.h ... don't have any AVAILABLE_IN guards in these files? > Need to update docs/ for this. Probably just adding soup-autocleanups.h to > IGNORE_HFILES is the right answer. Yup, sounds right to me. I don't think these need any docs.
Created attachment 310908 [details] [review] Support g_autoptr() for all libsoup object types This allows using e.g. g_autoptr(SoupSession) in other programs, but does not make libsoup itself use g_autoptr, or require a new enough glib to support it.
(In reply to Kalev Lember from comment #3) > > These ought to be SOUP_AVAILABLE_IN_2_52, though it seems like it's probably > > not possible to do that? Maybe wrap > > > > #if SOUP_VERSION_MAX_ALLOWED >= SOUP_VERSION_2_52 > > ... > > #endif > > > > around it? > > I don't know about that -- glib's and gtk's > /usr/include/glib-2.0/glib/glib-autocleanups.h > /usr/include/gtk-3.0/gtk/gtk-autocleanups.h > ... don't have any AVAILABLE_IN guards in these files? you can't use AVAILABLE_IN because of how the G_DEFINE_AUTOPTR_CLEANUP_FUNC macro expands, but I still think it would be best to put the version check above around the whole thing, so that people don't accidentally use the macros when they meant to depend on an older version of libsoup. OK to commit with that addition
(In reply to Dan Winship from comment #5) > so that people don't accidentally use the macros when they meant to depend on an older version of libsoup. Thanks, makes sense now why it's needed.
Thanks, pushed with that fix. Attachment 310908 [details] pushed as 5df74a1 - Support g_autoptr() for all libsoup object types
Reopening; I got a few free functions wrong in the first patch and found a more types that could be added.
Created attachment 311056 [details] [review] autocleanups: Fix free functions for non-GObject based types
Created attachment 311057 [details] [review] autocleanups: Add a few more missing types
Created attachment 311058 [details] [review] autocleanups: Exclude the header from instrospection
Review of attachment 311056 [details] [review]: ::: libsoup/soup-autocleanups.h @@ +23,2 @@ #include <libsoup/soup-types.h> +#include <libsoup/soup-uri.h> Which is better: include all the headers that we need here like I've done in this patch, or maybe alternatively move soup-autocleanups.h as the last include in soup.h? The last option is what glib/gtk+ are doing, but they are also enforcing single includes, so not sure which way is best here in libsoup.
Comment on attachment 311057 [details] [review] autocleanups: Add a few more missing types >+G_DEFINE_AUTOPTR_CLEANUP_FUNC(SoupMessageQueue, g_object_unref) That's an internal type (and not a GObject) >+G_DEFINE_AUTOPTR_CLEANUP_FUNC(SoupProxyResolver, g_object_unref) >+G_DEFINE_AUTOPTR_CLEANUP_FUNC(SoupProxyResolverDefault, g_object_unref) >+G_DEFINE_AUTOPTR_CLEANUP_FUNC(SoupProxyURIResolver, g_object_unref) Don't define autocleanups for deprecated types please. (There may be others in the list that I've missed too...)
(In reply to Kalev Lember from comment #12) > Which is better: include all the headers that we need here like I've done in > this patch, or maybe alternatively move soup-autocleanups.h as the last > include in soup.h? The last option is what glib/gtk+ are doing, but they are > also enforcing single includes, so not sure which way is best here in > libsoup. Either way is fine by me... while we don't *officially* require single-includes, IMHO including only <libsoup/soup.h> is still preferred, so semi-requiring that for new headers is fine.
Attachment 311056 [details] pushed as 0b0d6ab - autocleanups: Fix free functions for non-GObject based types Attachment 311058 [details] pushed as 4694333 - autocleanups: Exclude the header from instrospection
Okay, here's an updated patch with deprecated types removed:
Created attachment 311300 [details] [review] autocleanups: Add a few more missing types
Created attachment 311355 [details] [review] Require single includes for new headers For new headers added this cycle, enforce single includes by only allowing directly including <libsoup/soup.h>.
pushed those two patches. thanks