GNOME Bugzilla – Bug 788174
Use g_auto*
Last modified: 2018-02-12 12:49:42 UTC
We should use g_auto* instead of manually freeing resources: https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-auto Please don't put more than a single class in a single patch. It might be better to split bigger classes into multiple patches, but not more than one class per patch. Use your judgement.
Created attachment 361114 [details] [review] done-notification: Use g_auto*
Created attachment 361115 [details] [review] delete-notification: Use g_auto*
Created attachment 361116 [details] [review] source-notification: Use g_auto*
Created attachment 361117 [details] [review] indexing-notification: Use g_auto*
Created attachment 361118 [details] [review] print-notification: Use g_auto*
Created attachment 361119 [details] [review] share-notification: Use g_auto*
Created attachment 361120 [details] [review] image-view: Use g_auto*
Created attachment 361122 [details] [review] source-notification: Use g_auto*
Created attachment 361123 [details] [review] main-toolbar: Use g_auto*
Review of attachment 361122 [details] [review]: ::: src/photos-source-notification.c @@ -87,3 @@ g_warning ("Unable to launch gnome-control-center: %s", error->message); g_error_free (error); - goto out; You have to replace `goto out;` with `return;` if you want to maintain same control flow. @@ -106,3 @@ g_warning ("Unable to launch gnome-control-center: %s", error->message); g_error_free (error); - goto out; Here it is not strictly necessary, but I would add a `return;` anyway.
> You have to replace `goto out;` with `return;` if you want to maintain same > control flow. Hey thanks. I missed to notice that. I'll quickly update the patch. > > @@ -106,3 @@ > g_warning ("Unable to launch gnome-control-center: %s", > error->message); > g_error_free (error); > - goto out; > > Here it is not strictly necessary, but I would add a `return;` anyway.
Created attachment 361129 [details] [review] source-notification: Use g_auto*
Created attachment 361146 [details] [review] export-dialog: Use g_auto*
Created attachment 361148 [details] [review] indexing-notification: Use g_auto*
Created attachment 361149 [details] [review] source-notification: Use g_auto*
Created attachment 361193 [details] [review] properties-dialog: Use g_auto*
Created attachment 361254 [details] [review] facebook-item: Use g_auto*
Created attachment 361582 [details] [review] base-model: Use g_auto*
Created attachment 361612 [details] [review] empty-results-box: Use g_auto*
Created attachment 361614 [details] [review] error-box: Use g_auto*
Created attachment 361618 [details] [review] media-server-item: Use g_auto*
Review of attachment 361114 [details] [review]: Perfect. Thanks for the patch.
Review of attachment 361115 [details] [review]: ++
Review of attachment 361582 [details] [review]: Thanks for the patches, Ekta! One minor issue: ::: src/photos-base-model.c @@ +79,3 @@ photos_base_model_refresh (PhotosBaseModel *self) { + g_autoptr (GMenu) section; We should initialize these to NULL. I have seen GCC warning about the lack of initializers. Not sure why it's not warning during the build here. Could be due to a missing flag or something. @@ +97,3 @@ { + g_autoptr (GMenuItem) menu_item; + g_autoptr (GObject) object; Ditto.
Created attachment 362209 [details] [review] base-model: Use g_auto* Fixed and pushed.
Review of attachment 361118 [details] [review]: ++
Review of attachment 361193 [details] [review]: ::: src/photos-properties-dialog.c @@ +97,3 @@ PhotosPropertiesDialog *self; + g_autoptr (GError) error; + g_autofree gchar *camera; We should initialize them to NULL. @@ +128,3 @@ { PhotosPropertiesDialog *self; + g_autoptr (GError) error; Ditto. @@ -146,3 @@ g_warning ("Unable to resolve latitude and longitude: %s", error->message); - g_error_free (error); - goto out; I'd still like to retain the 'out' labels because they ensure that there's only one return site. Having a single return site is nice because (a) it helps when debugging, (b) we can add g_return_* assertions to enforce post-conditions. @@ -163,3 @@ - out: - g_clear_object (&place); - g_free (location_str); out: return; /* in case it warns about an empty label */
Review of attachment 361614 [details] [review]: ::: src/photos-error-box.c @@ +105,3 @@ photos_error_box_update (PhotosErrorBox *self, const gchar *primary, const gchar *secondary) { + g_autofree gchar *markup; This should be moved inside the braces below. @@ -116,3 @@ if (secondary != NULL) { markup = g_markup_escape_text (secondary, -1); Otherwise, if both primary and secondary are non-NULL, we'd be leaking the previous chunk of memory here.
Created attachment 362211 [details] [review] error-box: Use g_auto* Fixed and pushed.
I went through some of the patches. It would be nice if you could double-check the remaining ones and update them as per the above comments. Thanks for your contributions. Excited to see g_auto* adoption.
Created attachment 362991 [details] [review] properties-dialog: Use g_auto*
Review of attachment 362991 [details] [review]: Thanks for the patch, Ekta. Some comments below: ::: src/photos-properties-dialog.c @@ +96,3 @@ { PhotosPropertiesDialog *self; + g_autoptr (GError) error = NULL; For GErrors, it would be nice to tie them more tightly to the error sites. We can do so by using a pair of braces to narrow down the scope of the fallible call. That would address the following two issues with GError handling: (a) Forgetting to NULLify the GError before using it. (b) Leaking the GError. @@ +127,3 @@ { PhotosPropertiesDialog *self; + g_autoptr (GError) error = NULL; Ditto. @@ +214,3 @@ PhotosPropertiesDialog *self = PHOTOS_PROPERTIES_DIALOG (user_data); TrackerSparqlConnection *connection = TRACKER_SPARQL_CONNECTION (source_object); TrackerSparqlCursor *cursor = NULL; Cases like these where an external library doesn't have autocleanup definitions should be marked with a TODO and a bug filed. That will make it easier to mop them up later. @@ +348,3 @@ PhotosPropertiesDialog *self = PHOTOS_PROPERTIES_DIALOG (object); GApplication *app; GDateTime *date_modified; g_autoptr (...) date_modified = NULL; @@ +377,3 @@ const gchar *type_description; + g_autofree gchar *date_created_str = NULL; + g_autofree gchar *date_modified_str; Missing NULL initialization. @@ +648,3 @@ { GtkWidget *dims_data; + g_autofree gchar *dims_str; Ditto.
Created attachment 363060 [details] [review] properties-dialog: Use g_auto* Fixed and pushed.
Created attachment 363061 [details] [review] export-dialog: Use g_auto*
Review of attachment 361119 [details] [review]: ::: src/photos-share-notification.c @@ +97,3 @@ GtkWidget *image; GtkWidget *label; + g_autofree gchar *msg; Should be NULL initialized. See comment 24.
Review of attachment 361120 [details] [review]: ::: src/photos-image-view.c @@ +119,3 @@ { const Babl *format; + g_autoptr (GeglBuffer) buffer; Should be NULL initialized. See comment 24. @@ +307,3 @@ { GdkFrameClock *frame_clock; + g_autoptr (PhotosImageViewHelper) helper; Ditto. @@ +1052,3 @@ { GdkFrameClock *frame_clock; + g_autoptr (PhotosImageViewHelper) helper; Ditto.
Review of attachment 361254 [details] [review]: ::: src/photos-facebook-item.c @@ +82,3 @@ { PhotosFacebookItem *self = PHOTOS_FACEBOOK_ITEM (item); + g_autoptr (GDateTime) date_modified; Should be NULL initialized. See comment 24. @@ +135,1 @@ GFBGraphPhoto *photo = NULL; Should be marked with a TODO, and a bug filed against gfbgraph that blocks this one. Just like we did for tracker/TrackerSparqlCursor. See comment 32. @@ +192,1 @@ GFBGraphPhoto *photo = NULL; Ditto. @@ +263,3 @@ photos_facebook_item_open (PhotosBaseItem *item, GtkWindow *parent, guint32 timestamp) { + g_autoptr (GError) error; Would be nice to move the GError closer to the fallible call. See the committed patches for examples.
Comment on attachment 361146 [details] [review] export-dialog: Use g_auto* Oops! I failed to see that you had ported ExportDialog too, and I committed one separately. Sorry about this.
Review of attachment 361123 [details] [review]: Grepping for g_object_unref yields one use of GtkBuilder that should be g_auto-fied. A few other comments: ::: src/photos-main-toolbar.c @@ +91,1 @@ label = NULL; Nitpick: ideal use-case for g_steal_pointer. :) @@ +194,3 @@ PhotosDlnaRenderer *renderer; GtkLabel *label; + g_autofree gchar *text; Should be NULL initialized. @@ +313,3 @@ { GMenu *menu; + g_autoptr (GtkBuilder) builder; Ditto. @@ +328,3 @@ { GMenu *section; + g_autofree gchar *label; Ditto. @@ +360,3 @@ { GtkWidget *image; + g_autofree gchar *favorite_label; Ditto.
Created attachment 363065 [details] [review] main-toolbar: Use g_auto*
Review of attachment 361148 [details] [review]: ::: src/photos-indexing-notification.c @@ +237,3 @@ { GApplication *app; + g_autoptr (GError) error; Would be nice to move it closer to the fallible statement. See comment 32.
Review of attachment 361149 [details] [review]: ::: src/photos-source-notification.c @@ +65,3 @@ { + g_autoptr (GAppInfo) app = NULL; + g_autoptr (GError) error; The GError should be moved closer to the fallible statement. @@ -109,3 @@ } - - out: I'd still like to retain the 'out' labels because they ensure that there's only one return site. See comment 27.
Review of attachment 361612 [details] [review]: ::: src/photos-empty-results-box.c @@ +58,3 @@ { + g_autoptr (GAppInfo) app = NULL; + g_autoptr (GError) error; The GError should be moved closer to the fallible statement. See comment 32. @@ +75,3 @@ { g_warning ("Unable to launch gnome-control-center: %s", error->message); + return ret_val; Better to use 'goto out' since we already have it, and a single return site is a nice thing to have. See comment 27. @@ +123,3 @@ GtkWidget *details; + g_autofree gchar *details_str; + g_autofree gchar *system_settings_href; Should be NULL initialized. @@ +156,3 @@ GtkWidget *image; GtkWidget *title_label; + g_autofree gchar *label; Ditto.
Review of attachment 361618 [details] [review]: ::: src/photos-media-server-item.c @@ +81,3 @@ photos_media_server_item_create_thumbnail (PhotosBaseItem *item, GCancellable *cancellable, GError **error) { + g_autoptr (GFile) file; Should be NULL initialized. @@ -156,3 @@ local_path = NULL; - out: Would be good to keep the 'out' so that we have a single return site.
Created attachment 363386 [details] [review] share-notification: Use g_auto*
Created attachment 363387 [details] [review] source-notification: Use g_auto*
Created attachment 363388 [details] [review] image-view: Use g_auto*
Created attachment 363389 [details] [review] indexing-notification: Use g_auto*
Created attachment 363397 [details] [review] facebook-item: Use g_auto*
Created attachment 363450 [details] [review] empty-results-box: Use g_auto*
Review of attachment 363386 [details] [review]: Thanks for the new set of patches, Umang. Grepping for _free turns up a few more places that need g_auto*.
Created attachment 363520 [details] [review] share-notification: Use g_auto* I took the liberty to update the patch to cover all suitable g_auto* candidates.
Review of attachment 363450 [details] [review]: Thanks for the new patches, Ekta. Looks good, except for a couple of nitpicks: ::: src/photos-empty-results-box.c @@ +68,3 @@ + { + g_autoptr (GError) error = NULL; + app = g_app_info_create_from_commandline ("gnome-control-center online-accounts", Nitpick: missing newline after variable definition. @@ +91,3 @@ + { + g_autoptr (GError) error = NULL; + g_app_info_launch (app, NULL, G_APP_LAUNCH_CONTEXT (ctx), &error); Nitpick: missing newline after variable definition.
Created attachment 363530 [details] [review] empty-results-box: Use g_auto* Fixed and pushed.
Review of attachment 363387 [details] [review]: Perfect.
Review of attachment 363397 [details] [review]: This conflicts with commit 9b72b3b20824f5fa. Would it be possible for you try and resolve it by rebasing on top of current master?
Review of attachment 363388 [details] [review]: This conflicts with commit 150ae46fcc9b54c3. Would it be possible for you try and resolve it by rebasing on top of current master?
Review of attachment 363389 [details] [review]: Looks good, thanks.
Created attachment 363553 [details] [review] image-view: use g_auto*
Created attachment 363647 [details] [review] facebook-item: Use g_auto*
Created attachment 363648 [details] [review] zoom-controls: Use g_auto*
Review of attachment 363553 [details] [review]: Thanks for rebasing it. Pushed.
Review of attachment 363647 [details] [review]: Thanks for rebasing the patch! ::: src/photos-facebook-item.c @@ +246,1 @@ local_path = NULL; We could slip in a g_steal_pointer here. @@ +282,1 @@ { Nitpick: these braces can be removed.
Created attachment 364110 [details] [review] facebook-item: Use g_auto* Made the above changes and pushed.
Review of attachment 363648 [details] [review]: ::: src/photos-zoom-controls.c @@ +153,3 @@ GdkVisual *visual; GdkWindow *parent_window; + g_autoptr (GdkWindow) window; Should be initialized to NULL.
Created attachment 364111 [details] [review] zoom-controls: Use g_auto*
Created attachment 364113 [details] [review] media-server-item: Use g_auto*
Created attachment 364447 [details] [review] overview-searchbar: Use g_auto*
Created attachment 364448 [details] [review] query-builder: Use g_auto*
Review of attachment 364447 [details] [review]: Perfect, thanks.
Created attachment 364540 [details] [review] gesture-zoom: Use g_auto*
Review of attachment 364448 [details] [review]: ::: src/photos-query-builder.c @@ +391,3 @@ const gchar *path; + g_autofree gchar *pictures_uri = NULL; + g_auto (GStrv) tracker_dirs; Nice, but I'd still NULL initialize it because ultimately it will g_strfreev will be involved.
Created attachment 364543 [details] [review] query-builder: Use g_auto*
Created attachment 364583 [details] [review] local-item: Use g_auto*
Review of attachment 364540 [details] [review]: ++
Created attachment 364637 [details] [review] gegl: Use g_auto*
Created attachment 364827 [details] [review] base-item: Use g_auto* (1/3)
Created attachment 364828 [details] [review] base-item: Use g_auto* (2/3)
Created attachment 364829 [details] [review] base-item: Use g_auto* (3/3)
Created attachment 364841 [details] [review] edit-palette-row: Use g_auto*
Created attachment 364842 [details] [review] edit-palette: Use g_auto*
Created attachment 364843 [details] [review] remote-display-manager: Use g_auto*
Created attachment 364844 [details] [review] print-operation: Use g_auto*
Created attachment 364845 [details] [review] update-mime-job: Use g_auto*
Created attachment 364846 [details] [review] utils: Use g_auto* (1/2)
Created attachment 364847 [details] [review] utils: Use g_auto* (2/2)
Review of attachment 364583 [details] [review]: ::: src/photos-local-item.c @@ +221,3 @@ const gchar *version_tag = "Xmp.gnome.photos-xmp-version"; + g_autofree gchar *path = NULL; + g_autofree gchar *shared_string = NULL; We should either split shared_string into shared_string_old and shared_string_new; or somehow limit it to a narrower scope. The current code replaces the string mid-way through the function, which can get confusing with g_autoptr. Limiting symbols to a narrower scope is generally the better option. @@ +277,3 @@ } shared_variant = g_variant_builder_end (&builder); The GVariantBuilder was getting leaked if we failed to parse the shared_string above. It's a pre-existing bug. We can use g_auto with GVariantBuilder in this patch by limiting it to a narrower scope.
Created attachment 364855 [details] [review] local-item: Use g_auto* I tried to make the above changes, but this patch is sufficiently big that it would be nice if you could review it.
Review of attachment 364637 [details] [review]: Thanks, Umang. Looks good to me.
Review of attachment 364841 [details] [review]: ++
Created attachment 364857 [details] [review] tool-filter-button: Use g_auto*
Created attachment 364859 [details] [review] photos-glib: Use g_auto*
Created attachment 364882 [details] [review] main: Use g_auto*
Created attachment 364896 [details] [review] pixbuf: Use g_auto*
Created attachment 364906 [details] [review] flickr-item: Use g_auto*
Created attachment 364909 [details] [review] header-bar: Use g_auto*
Review of attachment 364842 [details] [review]: ++
Review of attachment 364827 [details] [review]: Thanks for splitting src/photos-base-item.c into multiple patches! Some minor details: ::: src/photos-base-item.c @@ +424,1 @@ g_object_unref (emblemed_pixbuf); A few lines above, the GError can also be g_auto-fied. @@ +607,3 @@ + { + g_autoptr (GError) error = NULL; + if (priv->default_app != NULL) I'd move the GError inside the if branch. The idea is to bind each GError as tightly as possible to the fallible function. That way it's easier to ensure that GErrors are not leaked or reused without being emptied. @@ +613,3 @@ g_warning ("Unable to show URI %s: %s", priv->uri, error->message); + } + else Same thing here. A separate GError inside 'else'. @@ +821,3 @@ + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + { + goto out; With g_auto* in place, we can simplify these branches by moving the code inside 'else' to the outer level, and not having a separate else branch. Earlier, generally speaking, goto:s inside branches or loops were problematic, if the code inside the branch required its own clean-up. That's why we tried to keep things as linear as possible with separate if-else stanzas. That's no longer a problem. @@ +876,3 @@ + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + { + goto out; Same thing here. @@ +942,3 @@ + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + { + goto out; Ditto. @@ +998,1 @@ goto out; Ditto. @@ +1039,3 @@ + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + { + goto out; Ditto.
Created attachment 364919 [details] [review] base-item: Use g_auto* (1/3) Made the above changes and pushed to master.
Review of attachment 364844 [details] [review]: ++
Review of attachment 364828 [details] [review]: Looks good, apart from: ::: src/photos-base-item.c @@ -1504,3 @@ g_object_ref (task)); - g_free (uri); Nitpick: stray newline left behind. @@ -1558,3 @@ g_object_ref (task)); - g_object_unref (task); Ditto.
Created attachment 364950 [details] [review] base-item: Use g_auto* (2/3)
Created attachment 365001 [details] [review] thumbnail-factory: Use g_auto*
Created attachment 365002 [details] [review] searchbar: Use g_auto*
Review of attachment 364843 [details] [review]: ::: src/photos-remote-display-manager.c @@ +173,3 @@ Share *share = user_data; PhotosDlnaRenderer *renderer = PHOTOS_DLNA_RENDERER (source_object); + g_autoptr (PhotosBaseItem) item = NULL; /* We already hold a ref to the item to be shared */ I'd drop this comment. It made some sense with the g_object_unref call, but now the "already" doesn't sound right any more. :) Plus, it's a bit obvious from the photos_dlna_renderer_share_finish_call.
Review of attachment 364859 [details] [review]: ++
Created attachment 365012 [details] [review] remote-display-manager: Use g_auto*
Created attachment 365100 [details] [review] fetch-ids-job: Use g_auto*
Review of attachment 364845 [details] [review]: Perfect, thanks.
Created attachment 365185 [details] [review] glib: Use g_auto* with custom closures
Created attachment 365225 [details] [review] tracker-change-monitor: Use g_auto*
Review of attachment 364829 [details] [review]: Thanks for the steady stream of patches. Some minor niggles: ::: src/photos-base-item.c @@ +2757,3 @@ + { + g_autoptr (GError) error = NULL; + node = photos_base_item_load_finish (self, res, &error); Nitpick: missing newline. @@ +2772,3 @@ + { + g_autoptr (GError) error = NULL; + print_res = gtk_print_operation_run (print_op, GTK_PRINT_OPERATION_ACTION_PRINT_DIALOG, toplevel, &error); Ditto. @@ -3973,3 +3964,2 @@ g_object_ref (task)); - g_object_unref (task); Nitpick: extra newline left behind.
Created attachment 365481 [details] [review] base-item: Use g_auto* (3/3)
Review of attachment 364857 [details] [review]: ::: src/photos-tool-filter-button.c @@ +112,3 @@ GtkWidget *image; PhotosWidgetShader *shader; cairo_surface_t *preview_icon_surface = NULL; We should add a TODO for cairo_surface_t.
Created attachment 365484 [details] [review] tool-filter-button: Use g_auto*
Review of attachment 364846 [details] [review]: ::: src/photos-utils.c @@ +223,2 @@ cairo_surface_destroy (surface); cairo_destroy (cr); We should mark these as TODO. @@ +292,2 @@ cairo_surface_t *icon_surface = NULL; cairo_surface_t *surface; We should mark these as TODO. @@ +851,3 @@ GError **error) { GFileAttributeMatcher *matcher = NULL; Ditto, and the GEnumClass further above.
Created attachment 365493 [details] [review] utils: Use g_auto* (1/2)
Review of attachment 364882 [details] [review]: This is trickier than the rest: ::: src/photos-main.c @@ -72,3 @@ exit_status = g_application_run (app, argc, argv); - g_object_unref (remote_display_mngr); - g_object_unref (app); We need to move most of main() into a smaller sub-scope. The EggCounterArena is meant to be used once everything has been unreffed or destroyed, so that it can report any (instrumented) type that still has instances flying around. So, I'd move all of main() into a sub-scope except the following two statements.
Review of attachment 364847 [details] [review]: ::: src/photos-utils.c @@ -1117,3 @@ - { - g_free (result); - result = g_ascii_strdown (extensions[i], -1); This doesn't look right. Since result is defined in the outer scope, it won't be freed after every iteration of the for loop. So, not freeing the result before assigning another memory address to it, means that we are losing the pointer to the earlier buffer and leaking it. This is an example where C and g_auto* doesn't match C++'s smart pointers. One way to avoid the g_free would be run the loop backwards, but that would be too much churn for this g_auto* patchset. So I think we'll have to live with the g_free for the time being. We can fix it up in a later patch/bug. @@ +1161,1 @@ gchar *uri = NULL; uri should be marked with g_autofree, not path. @@ +1363,3 @@ photos_utils_update_executed (GObject *source_object, GAsyncResult *res, gpointer user_data) { + TrackerSparqlConnection *connection = TRACKER_SPARQL_CONNECTION (source_object); /* TODO: g_autoptr */ This TODO isn't necessary because we don't need g_autoptr here. @@ +1388,3 @@ + { + g_autoptr (GError) error = NULL; + queue = photos_tracker_queue_dup_singleton (NULL, &error); Nitpick: missing newline. @@ +1398,1 @@ photos_tracker_queue_update (queue, query, NULL, photos_utils_update_executed, g_strdup (urn), g_free); With the push to g_auto*, I'd sneak in a g_steal_pointer to avoid the g_strdup. @@ +1428,1 @@ photos_tracker_queue_update (queue, query, NULL, photos_utils_update_executed, g_strdup (urn), g_free); Ditto.
> @@ +1398,1 @@ > photos_tracker_queue_update (queue, query, NULL, > photos_utils_update_executed, g_strdup (urn), g_free); > > With the push to g_auto*, I'd sneak in a g_steal_pointer to avoid the > g_strdup. > > @@ +1428,1 @@ > photos_tracker_queue_update (queue, query, NULL, > photos_utils_update_executed, g_strdup (urn), g_free); > > Ditto. hmm, The urn is a <const gchar *> type if you notice. As I understand, the ownership transfer using g_steal_pointer doesn't make sense to me. Can you explain?
Created attachment 365780 [details] [review] main: Use g_auto*
(In reply to Umang Jain from comment #120) > > @@ +1398,1 @@ > > photos_tracker_queue_update (queue, query, NULL, > > photos_utils_update_executed, g_strdup (urn), g_free); > > > > With the push to g_auto*, I'd sneak in a g_steal_pointer to avoid the > > g_strdup. > > > > @@ +1428,1 @@ > > photos_tracker_queue_update (queue, query, NULL, > > photos_utils_update_executed, g_strdup (urn), g_free); > > > > Ditto. > > hmm, The urn is a <const gchar *> type if you notice. As I understand, the > ownership transfer using g_steal_pointer doesn't make sense to me. Can you > explain? Oh, never mind. You're right. I was mistaken. Please ignore those two comments. Sorry about the confusion.
Created attachment 365785 [details] [review] utils: Use g_auto* (2/2)
Created attachment 365786 [details] [review] utils: Use g_auto* (2/2)
Created attachment 365787 [details] [review] utils: Use g_auto* (2/2)
Created attachment 365789 [details] [review] fetch-ids-job: Use g_auto*
Created attachment 365791 [details] [review] thumbnail-factory: Use g_auto*
Created attachment 365792 [details] [review] tracker-change-monitor: Use g_auto*
Created attachment 365793 [details] [review] flickr-item: Use g_auto*
Review of attachment 364896 [details] [review]: ++
Review of attachment 364909 [details] [review]: Thanks, Umang. Looks good to me.
Review of attachment 365002 [details] [review]: ++
Review of attachment 365780 [details] [review]: Perfect, looks good to me.
Review of attachment 365787 [details] [review]: Perfect, thanks.
Created attachment 366137 [details] [review] fetch-ids-job: Use g_auto* Pushed with some cosmetic fixes.
Review of attachment 365791 [details] [review]: ::: src/photos-thumbnail-factory.c @@ +404,1 @@ if (self->connection == NULL) The g_clear_error further above can now be avoided. @@ +407,2 @@ const gchar *address; + g_autofree gchar *thumbnailer_path = NULL; Pre-existing bug: the thumbnailer_path was being leaked in the error case.
Created attachment 366138 [details] [review] thumbnail-factory: Use g_auto*
Review of attachment 365792 [details] [review]: ++ ::: src/photos-tracker-change-monitor.c @@ +185,1 @@ PhotosTrackerChangeMonitorQueryData *data = (PhotosTrackerChangeMonitorQueryData *) user_data; Might be good to define a cleanup function for this ... @@ +234,3 @@ + { + g_warning ("Unable to resolve item URNs for graph changes: %s", error->message); + photos_tracker_change_monitor_query_data_free (data); ... so that we can get rid of this. It can be done in a separate patch, though. See commit e31095dcd81e786b.
Review of attachment 365793 [details] [review]: ::: src/photos-flickr-item.c @@ +145,3 @@ photos_flickr_item_create_thumbnail (PhotosBaseItem *item, GCancellable *cancellable, GError **error) { PhotosFlickrItemSyncData data; Might be nice to define a cleanup function. @@ +151,1 @@ GMainContext *context = NULL; Missing g_autoptr. @@ +151,3 @@ GMainContext *context = NULL; GrlMedia *media = NULL; GrlOperationOptions *options = NULL; Need some TODOs here. :)
Created attachment 366160 [details] [review] flickr-item: Use g_auto* I took the liberty to make the above adjustments and pushed to master.
Created attachment 366681 [details] [review] source-manager: Use g_auto*
Created attachment 366894 [details] [review] build: Generate autocleanup definitions for GDBus interfaces
Created attachment 367155 [details] [review] application: Use g_auto*
-- 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/gnome-photos/issues/77.