GNOME Bugzilla – Bug 752050
Fix -Werror build for clang
Last modified: 2015-07-08 16:09:52 UTC
The attached patch fixes cast alignment error, missing noreturn, uninitialized variable, and prevents using the value that does not exist in enum. There is still a problem that is not fixed by this patch: ephy-bookmarks-editor.c:1572:10: error: implicit conversion from enumeration type 'EphyBookmarkProperty' to different enumeration type 'EphyNodeViewPriority' [-Werror,-Wenum-conversion] EPHY_NODE_KEYWORD_PROP_PRIORITY); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. I hope someone can help fixing it.
Created attachment 306978 [details] [review] [PATCH] build: Fix -Werror build for clang
Review of attachment 306978 [details] [review]: Thanks. For that last warning, I would simply cast the EphyBookmarkProperty EPHY_NODE_KEYWORD_PROP_PRIORITY to an EphyNodeViewPriority. (Presumably clang's warnings go away if you add an explicit cast, right?) Note: a lot of GNOME packages are going to be turning on -Werror for builds from git. If you're running continuous integration, you probably want to configure with --disable-Werror to avoid this. If you're using jhbuild, a newer jhbuild will do this for you automatically. ::: src/bookmarks/ephy-bookmarks-editor.c @@ +402,3 @@ else if (ephy_node_view_is_target (EPHY_NODE_VIEW (editor->priv->key_view))) { + int priority; I think it would be more clear if you instead cast the return value to EphyNodePriority. @@ +1127,3 @@ { EphyNode *node = selected->data; + int priority; Same ::: src/bookmarks/ephy-bookmarks-export.c @@ +75,3 @@ { EphyNode *kid; + int priority; Same
Created attachment 307026 [details] [review] [PATCH] build: Fix -Werror build for clang (In reply to Michael Catanzaro from comment #2) > Review of attachment 306978 [details] [review] [review]: > > Thanks. > > For that last warning, I would simply cast the EphyBookmarkProperty > EPHY_NODE_KEYWORD_PROP_PRIORITY to an EphyNodeViewPriority. (Presumably > clang's warnings go away if you add an explicit cast, right?) > Added in the new patch. I just wonder whether it is the correct thing to do. > Note: a lot of GNOME packages are going to be turning on -Werror for builds > from git. If you're running continuous integration, you probably want to > configure with --disable-Werror to avoid this. If you're using jhbuild, a > newer jhbuild will do this for you automatically. > I know --disable-Werror, but I still try to fix the error if it is easy to fix.
Review of attachment 307026 [details] [review]: ::: src/bookmarks/ephy-bookmarks-editor.c @@ +409,3 @@ selected = ephy_node_view_get_selection (EPHY_NODE_VIEW (editor->priv->key_view)); node = selected->data; + priority_return = ephy_node_get_property_int (node, EPHY_NODE_KEYWORD_PROP_PRIORITY); Hm, this is too much work to convert an int to an enum. (Same for the other two places.) I was thinking to do this: priority = (EphyNodePriority)ephy_node_get_property_int (node, (EphyNodePriority)EPHY_NODE_KEYWORD_PROP_PRIORITY); Would that not silence the warning? If not, then we should just remove this warning when calling AX_COMPILER_FLAGS is configure.ac.
(In reply to Michael Catanzaro from comment #4) > I was thinking to do this: > > priority = (EphyNodePriority)ephy_node_get_property_int (node, > (EphyNodePriority)EPHY_NODE_KEYWORD_PROP_PRIORITY); > > Would that not silence the warning? If not, then we should just remove this > warning when calling AX_COMPILER_FLAGS is configure.ac. No, the warning is: ephy-bookmarks-editor.c:413:16: error: comparison of constant -1 with expression of type 'EphyNodePriority' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
OK, I think the warning is trying to tell us is that it's wrong to assume the enum is signed. Both of your patches fix that, but I think the first patch was better. I don't really like using an int instead of the more specific type, but that's probably the best approach until we can kill EphyNode. So the first patch is fine. For the warning you mention in comment #0, this should work: priority = ephy_node_get_property_int (node, (EphyNodePriority)EPHY_NODE_KEYWORD_PROP_PRIORITY);
Created attachment 307046 [details] [review] [PATCH] build: Fix -Werror build for clang This patch should fix all warnings for clang.
Review of attachment 307046 [details] [review]: Thanks, push after trivial fixes: ::: embed/ephy-embed-prefs.c @@ +364,3 @@ ephy_langs_sanitise (array); + webkit_web_context_set_preferred_languages (web_context, (const char * const *)(void*)array->data); When I looked at this yesterday I failed to notice or forgot to mention that we leave a space before the asterisk: (void *) @@ +368,2 @@ if (g_settings_get_boolean (EPHY_SETTINGS_WEB, EPHY_PREFS_WEB_ENABLE_SPELL_CHECKING)) { + char **normalized = normalize_languages ((char **)(void*)array->data); Here too ::: lib/ephy-langs.c @@ +148,3 @@ ephy_langs_sanitise (array); + return (char **)(void*) g_array_free (array, FALSE); And here
Modified patch pushed to master as 9f00bb5.