GNOME Bugzilla – Bug 789543
Simplify code by leveraging modern GLib API like g_clear_* and g_set_object, and dropping redundant NULL checks
Last modified: 2017-10-30 07:08:35 UTC
See patches. I would have pushed them directly but there are too big for me to be sure that I didn't break anything.
Created attachment 362386 [details] [review] operations: Simplify code
Created attachment 362388 [details] [review] gegl: Simplify code
Review of attachment 362386 [details] [review]: LGTM. Some nitpick space inconsistencies. ::: operations/external/jpg-load.c @@ +308,2 @@ g_object_unref(stream); + g_clear_object(&file); Space before "(" ::: operations/external/png-load.c @@ +435,3 @@ result.height = height; + g_clear_object(&infile); Ditto. @@ +467,3 @@ return FALSE; } + g_clear_object(&infile); Ditto. ::: operations/external/tiff-load.c @@ +297,3 @@ p->position = 0; + g_clear_pointer(&p->buffer, g_free); Ditto. @@ +846,3 @@ { cleanup(GEGL_OPERATION(object)); + g_clear_pointer(&o->user_data, g_free); Ditto. ::: operations/external/tiff-save.c @@ +265,3 @@ p->position = 0; + g_clear_pointer(&p->buffer, g_free); Ditto. @@ +594,3 @@ cleanup(operation); + g_clear_pointer(&o->user_data, g_free); + g_clear_error(&error); Ditto. ::: operations/workshop/gradient-map.c @@ +170,3 @@ } + g_free(props->gradient); Ditto. @@ +181,3 @@ if (o->user_data) { GradientMapProperties *props = (GradientMapProperties *)o->user_data; + g_free(props->gradient); Ditto.
Review of attachment 362388 [details] [review]: ::: gegl/graph/gegl-pad.c @@ +82,3 @@ + g_clear_pointer (&self->param_spec, (GDestroyNotify) g_param_spec_unref); + g_free (self->name); Nitpick: Just being pendatic here, self->name is set to NULL after freeing (in the current version), so maybe use g_clear_pointer (&self->name, g_free) in this patch? Some code rules say that to reset the variable to NULL after freeing them, to avoid dangling pointer bugs. So use your discretion here. I am just politely pointing this out here. ::: gegl/module/geglmoduledb.c @@ +120,3 @@ + g_list_free (db->modules); + g_free (db->load_inhibit); Ditto, as above. ::: gegl/process/gegl-eval-manager.c @@ +59,3 @@ GeglEvalManager *self = GEGL_EVAL_MANAGER (self_object); + g_free (self->pad_name); Ditto, as above. ::: gegl/property-types/gegl-audio-fragment.c @@ +56,3 @@ for (i = 0; i < GEGL_MAX_AUDIO_CHANNELS; i++) { + g_clear_pointer (&audio->data[i], g_free); Yes, this is the style I am talking about in above comments.
(In reply to Umang Jain from comment #3) > Review of attachment 362386 [details] [review] [review]: > > LGTM. Some nitpick space inconsistencies. > > ::: operations/external/jpg-load.c > @@ +308,2 @@ > g_object_unref(stream); > + g_clear_object(&file); > > Space before "(" > > ::: operations/external/png-load.c > @@ +435,3 @@ > result.height = height; > > + g_clear_object(&infile); > > Ditto. These are all cases where code around the changes don't use space before the opening parenthesis. So I went with the local customs.
Thanks for taking a look. (In reply to Umang Jain from comment #4) > ::: gegl/graph/gegl-pad.c > @@ +82,3 @@ > > + g_clear_pointer (&self->param_spec, (GDestroyNotify) g_param_spec_unref); > + g_free (self->name); > > Nitpick: Just being pendatic here, self->name is set to NULL after freeing > (in the current version), so maybe use g_clear_pointer (&self->name, g_free) > in this patch? > > Some code rules say that to reset the variable to NULL after freeing them, > to avoid dangling pointer bugs. So use your discretion here. I am just > politely pointing this out here. This is the finalize virtual method, not dispose. Once a GObject is in finalize, nothing can ever access it again, which is why there is no need to reset things to NULL. This is unlike dispose which can be called multiple times during the destruction of an object to break reference loops, and there's a need to reset things.