GNOME Bugzilla – Bug 762648
Various build system and compiler warning fixes
Last modified: 2017-05-05 08:47:52 UTC
A cornucopia of fixes and improvements to the build system and various compiler warning fixes so Bijiben builds nicely on my machine.
Created attachment 322285 [details] [review] build: Remove gnome-common dependency from autogen.sh See https://wiki.gnome.org/Projects/GnomeCommon/Migration.
Created attachment 322286 [details] [review] build: Remove use of AM_GLIB_GNU_GETTEXT Bijiben is already using intltool; you cannot use both at once.
Created attachment 322287 [details] [review] build: Remove GNOME_DOC_PREPARE macro usage This macro was remove from gnome-common a long time ago, and relates to the antique GNOME documentation infrastructure. Bijiben already uses Yelp.
Created attachment 322288 [details] [review] build: Use AS_HELP_STRING instead of AC_HELP_STRING AC_HELP_STRING has been deprecated for a long time.
Created attachment 322289 [details] [review] build: Use LT_INIT instead of AM_PROG_LIBTOOL AM_PROG_LIBTOOL has been deprecated for a long time.
Created attachment 322290 [details] [review] build: Fix quoting in configure.ac
Created attachment 322291 [details] [review] build: Check for libm at configure time You cannot rely on -lm being the correct way to link the maths library, as it might be provided by libc on some systems.
Created attachment 322292 [details] [review] build: Tidy up glib-compile-resources usage in Makefile.am Use $(AM_V_GEN), calculate dependencies automatically, and use nodist_bijiben_SOURCES correctly to ensure generated sources are not distributed. This means the dist-hook can be removed.
Created attachment 322293 [details] [review] build: Remove explicit -Wall -g from AM_CFLAGS These are developer options which should be set by specific developers in their local CFLAGS environment variable, otherwise they enter production builds. The ultimate solution to this is to use AX_COMPILER_FLAGS to set the default set of compiler flags. See http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html.
Created attachment 322294 [details] [review] build: Use AC_PATH_PROG to look up gdbus-codegen
Created attachment 322295 [details] [review] src: Fix a large number of memory leaks There are still some remaining, but for a first pass of very basic startup and editing functionality, this has eliminated several memory leaks.
Created attachment 322296 [details] [review] src: Various const-correctness fixes This helps clarify the memory management in several places, but is by no means complete.
Created attachment 322297 [details] [review] biji-webkit-editor: Fix read of undefined memory If css is less than 1000 characters long, this starts reading undefined memory off the end of the array. Use the string length as the number of bytes to write out.
Created attachment 322298 [details] [review] src: Mark various internal functions as static This will provide slight optimisations when linking, but mostly just shuts up compiler warnings about functions being defined before they’re declared.
Created attachment 322299 [details] [review] src: Various const-correctness fixes This makes it clearer about which bits of code have ownership of which bits of memory, and also shut up compiler warnings about discarding const modifiers from variables.
Created attachment 322300 [details] [review] src: Move all declarations to the top of blocks This fixes C90 style, which makes it easier to see all the memory a function allocates or handles throughout its body.
Created attachment 322301 [details] [review] src: Add missing enum cases and default cases to switch statements This makes it more explicit about how each element in an enum is handled by the switch. In several of the switches here, it’s not immediately obvious what should be done to handle the new cases, hence a FIXME has been added. This is the power of -Wenums.
Created attachment 322302 [details] [review] src: Fix old-style function definitions func() means ‘a function called func which accepts an undefined set of parameters’; whereas func(void) means ‘a function called func which accepts zero parameters’. This allows optimised function calls (not really important here, but not bad to have), allows detection of bad behaviour (like passing parameters to these functions) and shuts up some compiler warnings.
Created attachment 322303 [details] [review] src: Fix variable shadowing The global properties array was shadowed by some local variables. Since it was only used in class_init(), just move it there.
Created attachment 322304 [details] [review] src: Ignore deprecation warnings about old GTK+ functions These functions both require non-trivial porting effort to move away from, since they have been replaced by CSS. Squash the compiler warning for now and replace it with a FIXME instead.
Created attachment 322305 [details] [review] build: Fix include path for libbiji subdirectory This was triggering a compiler warning (and was not being a particularly useful -I flag).
Created attachment 322306 [details] [review] src: Use #ifdef instead of #if ‘#if’ only works if the variable is defined in the preprocessor (to have value 0 or 1); ‘#ifdef’ works more conventionally.
Created attachment 322307 [details] [review] src: Remove unused functions
Created attachment 322308 [details] [review] biji-note-obj: Remove duplicate function declaration
Created attachment 322309 [details] [review] editor: Fix signed-vs-unsigned comparisons The return value of strlen() is a size_t (equivalently gsize).
Created attachment 322310 [details] [review] build: Add AX_COMPILER_FLAGS to standardise compiler warning flags See http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html.
Review of attachment 322290 [details] [review]: Besides, this patch doesn't apply on the latest master branch. Please update it, Thanks. ::: configure.ac @@ +23,3 @@ GETTEXT_PACKAGE=bijiben +AC_SUBST([GETTEXT_PACKAGE]) +AC_DEFINE_UNQUOTED([GETTEXT_PACKAGE],"$GETTEXT_PACKAGE",[GETTEXT package name]) There should be "[]" for "$GETTEXT_PACKAGE" too. So it's > AC_DEFINE_UNQUOTED([GETTEXT_PACKAGE],["$GETTEXT_PACKAGE"],[GETTEXT package name])
Review of attachment 322295 [details] [review]: Please update this patch, as it won't apply cleanly on master branch. Thanks.
(In reply to Jonathan Kang from comment #27) > Review of attachment 322290 [details] [review] [review]: > > Besides, this patch doesn't apply on the latest master branch. Please update > it, Thanks. Sorry, I don’t have time to do that and test it all. I think you’ll have to rebase and update the patches yourself.
Comment on attachment 322285 [details] [review] build: Remove gnome-common dependency from autogen.sh Pushed to master as commit 9a93421b25824dff19a1c2485461759654012ca4.
(In reply to Philip Withnall from comment #29) > > Sorry, I don’t have time to do that and test it all. I think you’ll have to > rebase and update the patches yourself. Okay. Got it.
Comment on attachment 322286 [details] [review] build: Remove use of AM_GLIB_GNU_GETTEXT Pushed to master as commit 4fadc068a21528b1e07970132648d4a067289929.
Comment on attachment 322287 [details] [review] build: Remove GNOME_DOC_PREPARE macro usage Pushed to master as commit 708a1304340ec89836f22d83c71125db411b7aa8.
Comment on attachment 322288 [details] [review] build: Use AS_HELP_STRING instead of AC_HELP_STRING Pushed to master as commit abe9205e1308cdba3aa1140482f1ee1596a9ed2a.
Comment on attachment 322289 [details] [review] build: Use LT_INIT instead of AM_PROG_LIBTOOL Pushed to master as commit f71d83f88f29b368504903c4cf13f4dd4ae8d119.
Comment on attachment 322290 [details] [review] build: Fix quoting in configure.ac Rebased and pushed to master as commit 4771f37fc0e0bc6d123fb5f6f6efefed025a0b27.
Comment on attachment 322291 [details] [review] build: Check for libm at configure time Rebased and pushed to master as commit 0989cbe04912ab0764c9e5f484b104e8a5243f1d.
Comment on attachment 322292 [details] [review] build: Tidy up glib-compile-resources usage in Makefile.am Rebased and pushed to master as commit 0248bf87164a23f8ff9cd80b31c25af11e92ca53.
Comment on attachment 322293 [details] [review] build: Remove explicit -Wall -g from AM_CFLAGS Rebased and pushed to master as commit e20e922964bcae3c35c5f5d5e0250a1eb6619020.
Comment on attachment 322294 [details] [review] build: Use AC_PATH_PROG to look up gdbus-codegen Pushed to master as commit 488efe9ae2b2e569860dd2cdf145ba8b421944aa.
Comment on attachment 322295 [details] [review] src: Fix a large number of memory leaks Rebased and pushed to master as commit 389bb2e29786739b4a9d0199896f070e4ce85cdb.
Comment on attachment 322296 [details] [review] src: Various const-correctness fixes Rebased and pushed to master as commit 471810c6a1bb9dd9825649520fe16228f0be0972.
Comment on attachment 322297 [details] [review] biji-webkit-editor: Fix read of undefined memory The code base has changed a lot and this patch won't apply.
Comment on attachment 322298 [details] [review] src: Mark various internal functions as static Rebased and pushed to master as commit 2ec0b0c4d8c3b273d37f2708e1c2bc7da418c07c.
I still have to check it into detail as the commit is quite large, but "src: Fix a large number of memory leaks" (commit 389bb2e29786739b4a9d0199896f070e4ce85cdb) crashes bijiben in my computer. This is the output of bijiben through the console: (bijiben:2598): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'BijiMemoProvider' (bijiben:2598): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'BijiProvider' [New Thread 0x7fffcd7fa700 (LWP 2619)] manager changed no controller sending the mass change (bijiben:2598): GLib-GObject-WARNING **: invalid unclassed pointer in cast to 'BijiMemoProvider' ** GLib:ERROR:../../../../glib/ghash.c:373:g_hash_table_lookup_node: assertion failed: (hash_table->ref_count > 0) Thread 1 "bijiben" received signal SIGABRT, Aborted. __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
Created attachment 350088 [details] 389bb2e29786739b4a9d0199896f070e4ce85cdb bug core dump Core dump of the bug that crashes bijiben produced by changes at commit 389bb2e29786739b4a9d0199896f070e4ce85cdb.
There is also another problem with "build: Tidy up glib-compile-resources usage in Makefile.am" (0248bf87164a23f8ff9cd80b31c25af11e92ca53) that produces the next message: (bijiben:31478): Gtk-WARNING **: Theme parsing error: <broken file>:1:0: Failed to import: The resource at “/org/gnome/bijiben/Adwaita.css” does not exist
Created attachment 350090 [details] [review] Fixed a typo when compiling resources This patch fixes the problem with resource loading from my last comment. It was just a typo.
Comment on attachment 350090 [details] [review] Fixed a typo when compiling resources Pushed to master as commit e5426be522533772a170a8d7aa5d64b40321ebf0 with some changes according to the original patch. Thanks.
I have been looking the 389bb2e29786739b4a9d0199896f070e4ce85cdb commit into detail. Although I still haven't been able to figure out what is causing bijiben to crash in my computer, I would like to make the following comments: Changes in src/bjb-controller.c: @@ -567,6 +567,9 @@ bjb_controller_apply_needle (BjbController *self) else update_controller_callback (result, self); + result = g_list_first (result); /* update_controller_callback() sorts it */ + g_list_free (result); + Result points to the list head, so there is no need to get first/head again, calling g_list_free should be enough. Changes in src/libbiji/biji-manager.c: @@ -128,7 +128,7 @@ on_provider_abort_cb (BijiProvider *provider, const BijiProviderInfo *info; info = biji_provider_get_info (provider); - g_hash_table_remove (self->priv->providers, (gpointer) info->unique_id); + g_hash_table_remove (self->priv->providers, info->unique_id); g_object_unref (G_OBJECT (provider)); } @@ -149,7 +149,8 @@ _add_provider (BijiManager *self, const BijiProviderInfo *info; info = biji_provider_get_info (provider); - g_hash_table_insert (self->priv->providers, (gpointer) info->unique_id, provider); + g_hash_table_insert (self->priv->providers, + (gpointer) info->unique_id, provider); g_signal_connect (provider, "loaded", G_CALLBACK (on_provider_loaded_cb), self); @@ -297,8 +300,8 @@ biji_manager_init (BijiManager *self) * - local files stored notes = "local" * - own cloud notes = account_get_id */ - - priv->providers = g_hash_table_new (g_str_hash, g_str_equal); + priv->providers = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, g_object_unref); } @@ -348,6 +351,8 @@ biji_manager_finalize (GObject *object) g_hash_table_destroy (manager->priv->items); g_hash_table_destroy (manager->priv->archives); + g_hash_table_unref (manager->priv->providers); + g_clear_object (&manager->priv->local_provider); G_OBJECT_CLASS (biji_manager_parent_class)->finalize (object); } Some details regarding these changes. Providers hash table is created as follows: priv->providers = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref); So we are creating the hash table with the following functions: hash_func = g_str_hash key_equal_func = g_str_equal key_destroy_func = g_free value_destroy_func = g_object_unref When a provider is added, it is added as follows: g_hash_table_insert (self->priv->providers, (gpointer) info->unique_id, provider); The unique_id field which is a const field has a cast to a non const pointer, as required by g_hash_table_insert because it could *destroy* the value. This should not happen. If for example, we are using an OwnCloud provider, when it's constructed, the GoA account id, which is obtained with goa_account_get_id and returns a const gchar *, is stored in the provider's unique_id field. The goa_account_get_id contract says, among others, "Do not free the returned value, it belongs to object.", but we are using g_free as key_destroy_func. This destroy function is going to be called in: - on_provider_abort_cb function when calling to g_hash_table_remove, calling g_free for info->unique_id and g_object_unref for provider. There is a duplicated g_object_unref for provider. - biji_manager_finalize function when calling to g_hash_table_unref for provider's variable, calling g_free for all the unique_ids on it used as keys. - _add_provider which I suppose shouldn't happen twice for any provider, but g_hash_table_insert could call g_free for unique_id. @@ -185,6 +186,7 @@ load_goa_client (BijiManager *self, g_message ("Loading account %s", goa_account_get_id (account)); provider = biji_own_cloud_provider_new (self, object); _add_provider (self, provider); + g_object_unref (provider); } } } @@ -204,6 +206,7 @@ load_eds_registry (BijiManager *self, { provider = biji_memo_provider_new (self, l->data); _add_provider (self, provider); + g_object_unref (provider); } g_list_free_full (list, g_object_unref); biji_memo_provider_new creates a new BijiProvider that is added to manager's providers variable/hash table by calling to _add_provider and then to g_hash_table_insert. As far as I know, g_hash_table_insert stores the pointer but does not reference the object, so when g_object_unref the provider object finalize method is called, memory is freed. Changes in src/libbiji/biji-tracker.c: @@ -339,6 +339,8 @@ biji_get_all_notebooks_async (BijiManager *manager, NULL); bjb_query_async (manager, query, cb, NULL, user_data); + + g_free (query); } It uses g_strconcat to concatenate all the strings for the query, but it's totally unnecessary because there is no variables, so the string concatenation can be replaced by a string literal (so there is no need to free it later). Changes in src/libbiji/biji-zeitgeist.c: @@ -111,6 +112,9 @@ check_insert_create_zeitgeist (BijiNoteObj *note) NULL, (GAsyncReadyCallback) on_find_create_event, note); + + g_ptr_array_unref (templates); + g_free (uri); } I'm not sure about this as zeitgeist documentation lacks information and my guesses are based on examples. Shouldn't the template's variable, which is passed as a parameter when calling g_ptr_array_unref, be created using g_ptr_array_new_with_free_func (g_object_unref)? Changes in src/libbiji/provider/biji-memo-provider.c: @@ -171,7 +171,7 @@ create_note_from_item (BijiMemoItem *item) biji_manager_get_default_color (manager, &color); biji_note_obj_set_rgba (note, &color); g_hash_table_replace (item->self->priv->items, biji_note_obj_set_rgba (note, &color); g_hash_table_replace (item->self->priv->items, - item->set.url, + g_strdup (item->set.url), note); } @@ -650,7 +652,8 @@ biji_memo_provider_init (BijiMemoProvider *self) priv = self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, BIJI_TYPE_MEMO_PROVIDER, BijiMemoProviderPrivate); priv->queue = g_queue_new (); - priv->items = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); + priv->items = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, g_object_unref); priv->tracker = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); priv->memos = NULL; Instead of copying item->set.url and use g_free as key_destroy_func, it would be better to use item->set.url without any key_destroy_func, so you could avoid copying and freeing it later.
(In reply to Iñigo Martínez from comment #50) > > Changes in src/bjb-controller.c: > > @@ -567,6 +567,9 @@ bjb_controller_apply_needle (BjbController *self) > else > update_controller_callback (result, self); > > + result = g_list_first (result); /* update_controller_callback() sorts > it */ > + g_list_free (result); > + > > Result points to the list head, so there is no need to get first/head again, > calling g_list_free should be enough. You're right. > > Changes in src/libbiji/biji-manager.c: > > @@ -128,7 +128,7 @@ on_provider_abort_cb (BijiProvider *provider, > const BijiProviderInfo *info; > > info = biji_provider_get_info (provider); > - g_hash_table_remove (self->priv->providers, (gpointer) info->unique_id); > + g_hash_table_remove (self->priv->providers, info->unique_id); > > g_object_unref (G_OBJECT (provider)); > } > @@ -149,7 +149,8 @@ _add_provider (BijiManager *self, > const BijiProviderInfo *info; > > info = biji_provider_get_info (provider); > - g_hash_table_insert (self->priv->providers, (gpointer) info->unique_id, > provider); > + g_hash_table_insert (self->priv->providers, > + (gpointer) info->unique_id, provider); This should be >g_hash_table_insert (self->priv->providers, > g_strdup (info->unique_id), g_object_ref (provider)); It should fix the related issue you mentioned. > > @@ -185,6 +186,7 @@ load_goa_client (BijiManager *self, > g_message ("Loading account %s", goa_account_get_id (account)); > provider = biji_own_cloud_provider_new (self, object); > _add_provider (self, provider); > + g_object_unref (provider); > } > } > } > @@ -204,6 +206,7 @@ load_eds_registry (BijiManager *self, > { > provider = biji_memo_provider_new (self, l->data); > _add_provider (self, provider); > + g_object_unref (provider); > } > > g_list_free_full (list, g_object_unref); > > biji_memo_provider_new creates a new BijiProvider that is added to manager's > providers variable/hash table by calling to _add_provider and then to > g_hash_table_insert. As far as I know, g_hash_table_insert stores the > pointer but does not reference the object, so when g_object_unref the > provider object finalize method is called, memory is freed. This should be fixed by the above change. > > Changes in src/libbiji/biji-tracker.c: > > @@ -339,6 +339,8 @@ biji_get_all_notebooks_async (BijiManager *manager, > NULL); > > bjb_query_async (manager, query, cb, NULL, user_data); > + > + g_free (query); > } > > It uses g_strconcat to concatenate all the strings for the query, but it's > totally unnecessary because there is no variables, so the string > concatenation can be replaced by a string literal (so there is no need to > free it later). Emm. Makes sense. I suppose the original author want to keep the consistency of the codes(the other parts uses g_strconcat to generate the string). Let'us keep it as what it is now. > > Changes in src/libbiji/biji-zeitgeist.c: > > @@ -111,6 +112,9 @@ check_insert_create_zeitgeist (BijiNoteObj *note) > NULL, > (GAsyncReadyCallback) on_find_create_event, > note); > + > + g_ptr_array_unref (templates); > + g_free (uri); > } > > I'm not sure about this as zeitgeist documentation lacks information and my > guesses are based on examples. Shouldn't the template's variable, which is > passed as a parameter when calling g_ptr_array_unref, be created using > g_ptr_array_new_with_free_func (g_object_unref)? Quite right. > > Changes in src/libbiji/provider/biji-memo-provider.c: > > @@ -171,7 +171,7 @@ create_note_from_item (BijiMemoItem *item) > biji_manager_get_default_color (manager, &color); > biji_note_obj_set_rgba (note, &color); > g_hash_table_replace (item->self->priv->items, > biji_note_obj_set_rgba (note, &color); > g_hash_table_replace (item->self->priv->items, > - item->set.url, > + g_strdup (item->set.url), > note); > } > @@ -650,7 +652,8 @@ biji_memo_provider_init (BijiMemoProvider *self) > priv = self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, > BIJI_TYPE_MEMO_PROVIDER, BijiMemoProviderPrivate); > > priv->queue = g_queue_new (); > - priv->items = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, > NULL); > + priv->items = g_hash_table_new_full (g_str_hash, g_str_equal, > + g_free, g_object_unref); > priv->tracker = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, > g_free); > priv->memos = NULL; > > Instead of copying item->set.url and use g_free as key_destroy_func, it > would be better to use item->set.url without any key_destroy_func, so you > could avoid copying and freeing it later. Duplicating it is reasonable, as we don't want to reply on item->set.url. What if item->set.url is free.
Created attachment 350174 [details] [review] Fix some memory leaks introduced by commit 389bb2e29786739b4a9d0199896f070e4ce85cdb @Iñigo Could you please take a look at this patch? Thanks.
Changes at src/bjb-controller.c and src/libbiji/biji-zeitgeist.c look fine. Regarding the biji-manager.c change. It does the following: - When biji_manager_initable_init is called, BijiManager gets the "connectors"/clients (client, registry) and calls to their loading functions[0]. - Then it creates every provider and adds them to the hash table, although the providers are not in a valid state, and connects to loaded or abort signals to figure it what happens with it. - If the provider loads properly, items are loaded into the manager. - If the provider aborts, it is removed from the hash table, where it shouldn't be added until we knew that the provider was valid, and the provider unreferenced/finalized. My approach would be as follows: - When biji_manager_initable_init is called, call to their loading functions[0], where the "connector"/client will be acquired, providers created and BijiManager will connect to their loaded/abort signal. - For every provider that loads properly, we add it to the manager's providers list as a valid provider list, and we add all the items to manager. These providers will stay until we call to g_hash_table_unref where they should be unreferenced/freed. - If the provider aborts, we just unreference/free it. We are using provider's information unique_id member as a key, which is a const pointer, and we don't have to free it. As a value, we are using the provider's reference that we are creating at loading time, so we have to free it. Following aboves approach, I would create the providers hash table as follows: priv->providers = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); And I would add them as follows, *only* when we know that it loaded properly and the provider will be used: g_hash_table_insert (self->priv->providers, (gpointer) info->unique_id, provider); For pure correctness, we should create a non-const pointer for unique_id, but it involves copying it and freeing it later. By creating the hash table without key_destroy_func, we know that hash table will not try to free unique_id, so we can just make a cast, saving copy and free cpu time. When the hash table is unreferenced/freed, just when BijiManager finalizes, it will call to g_object_unref with provider unreferencing/freeing it. [0] Probably I should get the "connectors"/clients inside their load_* functions, so every provider functions are isolated inside those functions.
(In reply to Iñigo Martínez from comment #53) > Changes at src/bjb-controller.c and src/libbiji/biji-zeitgeist.c look fine. > > Regarding the biji-manager.c change. It does the following: > > - When biji_manager_initable_init is called, BijiManager gets the > "connectors"/clients (client, registry) and calls to their loading > functions[0]. > - Then it creates every provider and adds them to the hash table, although > the providers are not in a valid state, and connects to loaded or abort > signals to figure it what happens with it. > - If the provider loads properly, items are loaded into the manager. > - If the provider aborts, it is removed from the hash table, where it > shouldn't be added until we knew that the provider was valid, and the > provider unreferenced/finalized. > > My approach would be as follows: I'll push my patch to master to fix the crash at the moment. You can do those changes later on. Is that okay?
Comment on attachment 322299 [details] [review] src: Various const-correctness fixes Rebased and pushed to master as commit e971a0b914ec407d644cce8bde213b2f91277ea3.
Comment on attachment 322300 [details] [review] src: Move all declarations to the top of blocks Rebased and pushed to master as commit 3f4faecf8f718ea79de61b5fd9dfb51eac6fe571.
Comment on attachment 322302 [details] [review] src: Fix old-style function definitions Pushed to master as commit c819c0c666e7a632f0dba8a3697deea56318600d.
Comment on attachment 322305 [details] [review] build: Fix include path for libbiji subdirectory Pushed to master as commit 6eda3573ee7d0066687f1863c31c999a2cb4ab18.
Comment on attachment 322306 [details] [review] src: Use #ifdef instead of #if Pushed to master as commit c1d660bf94c1a7e30064e3f96c9bff016cb14c29.
Comment on attachment 322308 [details] [review] biji-note-obj: Remove duplicate function declaration Pushed to master as commit da9a03e81b181bcb59c4fb9f67ea95ecdb5f296d.
Comment on attachment 322304 [details] [review] src: Ignore deprecation warnings about old GTK+ functions The gtk_widget_override_background_color deprecation has been fixed, whild the gtk_tree_view_set_rules_hint deprecation hasn't. But let's leave the deprecation there util it's fixed.
Comment on attachment 322309 [details] [review] editor: Fix signed-vs-unsigned comparisons Won't apply any more.
Comment on attachment 322307 [details] [review] src: Remove unused functions Pushed to master as commit f0a5155e2e6e4c121177fdf3caf9eb34b948e9d1.
Comment on attachment 350174 [details] [review] Fix some memory leaks introduced by commit 389bb2e29786739b4a9d0199896f070e4ce85cdb Pushed to master as commit 226c8314915e48cfc5414d99fcabde50cdb75ab5.
Review of attachment 322301 [details] [review]: diff --git a/src/bjb-controller.c b/src/bjb-controller.c index f23e718..f799f28 100644 --- a/src/bjb-controller.c +++ b/src/bjb-controller.c @@ -537,6 +537,10 @@ update_controller_callback (GList *result, bjb_window_base_switch_to (priv->window, BJB_WINDOW_BASE_ARCHIVE_VIEW); break; + case BIJI_LIVING_ITEMS: + /* FIXME: Does anything need to be done here? */ + break; + default: break; } @@ -669,6 +673,11 @@ on_manager_changed (BijiManager *manager, break; + case BIJI_MANAGER_CHANGE_FLAG: + case BIJI_MANAGER_MASS_CHANGE: + /* FIXME: Does anything need to be done here? */ + break; + /* Apply the needle to display the relevant items. * The window will ping to tell it's now active * Use another thread for this, because controller is now up to date, diff --git a/src/bjb-empty-results-box.c b/src/bjb-empty-results-box.c index decbf32..2f8aa3b 100644 --- a/src/bjb-empty-results-box.c +++ b/src/bjb-empty-results-box.c @@ -162,6 +162,10 @@ bjb_empty_results_box_set_type (BjbEmptyResultsBox *self, gtk_widget_show (self->priv->image); break; + case BJB_EMPTY_RESULTS_TYPE: + /* FIXME: Does anything need to be done here? */ + break; + default: break; } I would not add these cases it as I would try to maintain only those cases where something is done. The rest cases are covered by the default case, which by the way is not necessary. ISO/IEC 9899:1999, section 6.8.4.2: [...] If no converted case constant expression matches and there is no default label, no part of the switch body is executed. This said, imho, I would maintain the default case because it adds readability to the code. diff --git a/src/bjb-main-toolbar.c b/src/bjb-main-toolbar.c index 1b29f77..b639209 100644 --- a/src/bjb-main-toolbar.c +++ b/src/bjb-main-toolbar.c @@ -37,7 +37,6 @@ typedef enum BJB_TOOLBAR_TRASH_ICON, BJB_TOOLBAR_TRASH_SELECT, BJB_TOOLBAR_NOTE_VIEW, - BJB_TOOLBAR_NUM } BjbToolbarType; /* Color Button */ I do agree with this change. I would not maintain unused code so it's better to remove the unused enum value. @@ -1011,10 +1011,14 @@ populate_main_toolbar(BjbMainToolbar *self) break; - + case BJB_WINDOW_BASE_ERROR_TRACKER: + /* FIXME: Do we need to do anything here? */ + break; /* Not really a toolbar, * still used for Spinner */ + case BJB_WINDOW_BASE_SPINNER_VIEW: + case BJB_WINDOW_BASE_NO_VIEW: default: to_be = BJB_TOOLBAR_0; } The same as above, I should not add those lines. Their cases are covered by the default case because to_be is initialized to BJB_TOOLBAR_0. On the other hand, default case should not be there or just be a break, because it is also covered by the to_be variable initialization. +++ b/src/bjb-window-base.c @@ -556,7 +556,7 @@ bjb_window_base_switch_to (BjbWindowBase *self, BjbWindowViewType type) gtk_stack_set_visible_child_name (priv->stack, "note-view"); break; - + case BJB_WINDOW_BASE_NO_VIEW: default: return; } The same as above. diff --git a/src/libbiji/deserializer/biji-lazy-deserializer.c b/src/libbiji/deserializer/biji-lazy-deserializer.c index 848072c..0c36c3c 100644 --- a/src/libbiji/deserializer/biji-lazy-deserializer.c +++ b/src/libbiji/deserializer/biji-lazy-deserializer.c @@ -278,6 +278,10 @@ process_tomboy_node (BijiLazyDeserializer *self) case XML_DTD_NODE: process_tomboy_text_elem (self); break; + + default: + /* Ignore. */ + break; } } diff --git a/src/libbiji/deserializer/biji-tomboy-reader.c b/src/libbiji/deserializer/biji-tomboy-reader.c index 01ede3e..a2f332f 100644 --- a/src/libbiji/deserializer/biji-tomboy-reader.c +++ b/src/libbiji/deserializer/biji-tomboy-reader.c @@ -211,6 +211,10 @@ process_tomboy_node (BijiTomboyReader *self) case XML_DTD_NODE: process_tomboy_text_elem (self); break; + + default: + /* Ignore. */ + break; } } I would not make the change or I would include the default case without the comment. diff --git a/src/libbiji/editor/biji-editor-selection.c b/src/libbiji/editor/biji-editor-selection.c index e318323..9e6eeb3 100644 --- a/src/libbiji/editor/biji-editor-selection.c +++ b/src/libbiji/editor/biji-editor-selection.c @@ -322,6 +322,9 @@ e_editor_selection_get_property (GObject *object, g_value_set_boolean (value, e_editor_selection_get_underline (selection)); return; + + default: + g_assert_not_reached (); } G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); @@ -400,6 +403,9 @@ e_editor_selection_set_property (GObject *object, e_editor_selection_set_underline ( selection, g_value_get_boolean (value)); return; + + default: + g_assert_not_reached (); } G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); I don't think calling g_assert_not_reached, even if the macro could be turned off when compiling. G_OBJECT_WARN_INVALID_PROPERTY_ID handles the default case "properly". By the way, although the code (before the patch) is valid. I have never seen a *_get_property function written this way. Looking at other GNOME applications, they use a break on every switch case, and a default case with G_OBJECT_WARN_INVALID_PROPERTY_ID. This would be better as there would only a single return point.
(In reply to Iñigo Martínez from comment #65) *snip* > diff --git a/src/bjb-empty-results-box.c b/src/bjb-empty-results-box.c > index decbf32..2f8aa3b 100644 > --- a/src/bjb-empty-results-box.c > +++ b/src/bjb-empty-results-box.c > @@ -162,6 +162,10 @@ bjb_empty_results_box_set_type (BjbEmptyResultsBox > *self, > gtk_widget_show (self->priv->image); > break; > > + case BJB_EMPTY_RESULTS_TYPE: > + /* FIXME: Does anything need to be done here? */ > + break; > + > default: > break; > } > > I would not add these cases it as I would try to maintain only those cases > where something is done. The rest cases are covered by the default case, > which by the way is not necessary. Both the extra case and the default case are necessary if using -Wswitch-enum and -Wswitch-default. The advantage of putting them there is that it clarifies the *intent* of the code — it makes it obvious that the cases are handled (and handling them is a no-op) rather than being accidentally omitted. This becomes more helpful if another member is added to the enum, as then the compiler automatically warns about all the switch statements which need to be updated to handle it. > diff --git a/src/bjb-main-toolbar.c b/src/bjb-main-toolbar.c > index 1b29f77..b639209 100644 > --- a/src/bjb-main-toolbar.c > +++ b/src/bjb-main-toolbar.c > @@ -37,7 +37,6 @@ typedef enum > BJB_TOOLBAR_TRASH_ICON, > BJB_TOOLBAR_TRASH_SELECT, > BJB_TOOLBAR_NOTE_VIEW, > - BJB_TOOLBAR_NUM > } BjbToolbarType; > > /* Color Button */ > > I do agree with this change. I would not maintain unused code so it's better > to remove the unused enum value. I removed it because -Wswitch-enum would require a `case BJB_TOOLBAR_NUM` to be added to all switch statements for this enum, which would not be helpful. > @@ -1011,10 +1011,14 @@ populate_main_toolbar(BjbMainToolbar *self) > > break; > > - > + case BJB_WINDOW_BASE_ERROR_TRACKER: > + /* FIXME: Do we need to do anything here? */ > + break; > > /* Not really a toolbar, > * still used for Spinner */ > + case BJB_WINDOW_BASE_SPINNER_VIEW: > + case BJB_WINDOW_BASE_NO_VIEW: > default: > to_be = BJB_TOOLBAR_0; > } > > The same as above, I should not add those lines. Their cases are covered by > the default case because to_be is initialized to BJB_TOOLBAR_0. Same reasoning as above: -Wswitch-enum. > diff --git a/src/libbiji/deserializer/biji-tomboy-reader.c > b/src/libbiji/deserializer/biji-tomboy-reader.c > index 01ede3e..a2f332f 100644 > --- a/src/libbiji/deserializer/biji-tomboy-reader.c > +++ b/src/libbiji/deserializer/biji-tomboy-reader.c > @@ -211,6 +211,10 @@ process_tomboy_node (BijiTomboyReader *self) > case XML_DTD_NODE: > process_tomboy_text_elem (self); > break; > + > + default: > + /* Ignore. */ > + break; > } > } > > I would not make the change or I would include the default case without the > comment. The comment is there to make it more obvious that the case is deliberately empty, not a copy-paste error or accidental omission. > diff --git a/src/libbiji/editor/biji-editor-selection.c > b/src/libbiji/editor/biji-editor-selection.c > index e318323..9e6eeb3 100644 > --- a/src/libbiji/editor/biji-editor-selection.c > +++ b/src/libbiji/editor/biji-editor-selection.c > @@ -322,6 +322,9 @@ e_editor_selection_get_property (GObject *object, > g_value_set_boolean (value, > e_editor_selection_get_underline (selection)); > return; > + > + default: > + g_assert_not_reached (); > } > > G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); > @@ -400,6 +403,9 @@ e_editor_selection_set_property (GObject *object, > e_editor_selection_set_underline ( > selection, g_value_get_boolean (value)); > return; > + > + default: > + g_assert_not_reached (); > } > > G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); > > By the way, although the code (before the patch) is valid. I have never seen > a *_get_property function written this way. Looking at other GNOME > applications, they use a break on every switch case, and a default case with > G_OBJECT_WARN_INVALID_PROPERTY_ID. This would be better as there would only > a single return point. Agreed.
Created attachment 350354 [details] [review] note-view: segfault back button Fixed bug introduced in commit 389bb2e29786739b4a9d0199896f070e4ce85cdb which added code to clear editor-toolbar object in the finalization method of note-view. The problem is that, this object is already cleared by its parent container, when their parent window is destroyed.
Review of attachment 350354 [details] [review]: Wrong commit.
Created attachment 350357 [details] [review] note-view: segfault back button Fixed bug introduced in commit 389bb2e29786739b4a9d0199896f070e4ce85cdb which added code to clear editor-toolbar object in the finalization method of note-view. The problem is that, this object is already cleared by its parent container, when their parent window is destroyed.
Created attachment 350378 [details] [review] improvements on providers loading Loading of providers has been improved by isolating them. The manager's providers hash table behaviour has also been changed: - Keys are not freed anymore so there is no need to duplicate the providers info unique_id. - Values are unreferenced on the hash table finalization, so providers are handled gracefully. - Providers are inserted only when they are loaded so there is no need to remove them on abort, just unreference them in order to finalize them.
Comment on attachment 350378 [details] [review] improvements on providers loading This patch does not apply in this bug report.
Comment on attachment 350357 [details] [review] note-view: segfault back button This has been pushed to master as commit 257a48d87dde3347814ecf5a9beee0607334022d. Mark it as commited.
Created attachment 350522 [details] [review] src: Add missing cases to switch statements This makes it more explicit about how each element in an enum is handled by the switch. It also fixes some trailing whitespace.
Review of attachment 350522 [details] [review]: Thanks for your patch.
Review of attachment 322310 [details] [review]: Hi Philip, thanks again for your patch. Unfortunately it didn't apply to master. Can you please fix it? Thanks.
(In reply to Isaque Galdino from comment #75) > Review of attachment 322310 [details] [review] [review]: > > Hi Philip, thanks again for your patch. > > Unfortunately it didn't apply to master. > > Can you please fix it? Sorry, I don’t have time to update it.
Created attachment 351145 [details] [review] build: Add AX_COMPILER_FLAGS to standardise compiler warning flags See http://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html. https://bugzilla.gnome.org/show_bug.cgi?id=762648 Signed-off-by: Isaque Galdino <igaldino@gmail.com>
Review of attachment 351145 [details] [review]: Patch changed and committed ;)
(In reply to Isaque Galdino from comment #78) > Review of attachment 351145 [details] [review] [review]: > > Patch changed and committed ;) Thanks! I hope the patch set was useful in the end.