With this, you should be able to build *src/* and *plugins/* warning-free with `CFLAGS="-Wall -Wextra -g -Og -Wno-unused-parameter -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wnested-externs -Werror-implicit-function-declaration"` (or just `-Wall -Wextra -g -Og -Wno-unused-parameter -Wwrite-strings` to start with) but for some Lexilla prototype and one deprecated Lexilla call.
To review, go per-commit as it should be fairly trivial that way. Also, everything but maybe 12b30c5950eeccad2fed885745ff6cd0888d1d3a should not be controversial I think -- but please, raise any concern! You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3665
-- Commit Summary --
* Fix harmless GCC warning * Fix fairly nasty implicit integer conversion * "Fix" passing const arguments to spawn functions * Add casts for irrelevant -Wwrite-strings warnings * Remove invalid placeholder code * Use g_*list_free_full() instead of g_*list_foreach() * Avoid an easy-to-fix function cast * Silence warnings on appropriate GSourceFunc casts * Fix variable shadowing * Only pass literals as format strings for e.g. printf-style functions * Do not use GtkResponseType as response ID type * Silence some -Wcast-function-type warnings
-- File Changes --
M plugins/filebrowser.c (12) M plugins/splitwindow.c (2) M src/build.c (40) M src/dialogs.c (5) M src/document.c (27) M src/editor.c (2) M src/filetypes.c (3) M src/libmain.c (16) M src/notebook.c (14) M src/plugins.c (14) M src/printing.c (4) M src/search.c (10) M src/spawn.c (10) M src/symbols.c (2) M src/tagmanager/tm_ctags.c (2) M src/tagmanager/tm_workspace.c (7) M src/toolbar.c (5) M src/ui_utils.c (35) M src/utils.h (4) M src/vte.c (10)
-- Patch Links --
https://github.com/geany/geany/pull/3665.patch https://github.com/geany/geany/pull/3665.diff
@elextr approved this pull request.
Gold star to @b4n, one suggestion for extra points.
if (main_status.main_window_realized) - dialogs_show_msgbox(GTK_MESSAGE_WARNING, warn_msg, display_filename); + dialogs_show_msgbox(GTK_MESSAGE_WARNING, "%s", warn_msg); + + ui_set_statusbar(TRUE, "%s", warn_msg);
Maybe the format should be taken out of `ui_set_statusbar` and `msgwin_status_add` or the compiler told they are [formats](https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Function-Attributes.html) [and](https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#for...).
Note this is passed through printf substitution here, in `ui_set_statusbar()` and `msgwin_status_add()` called from that.
@b4n commented on this pull request.
if (main_status.main_window_realized) - dialogs_show_msgbox(GTK_MESSAGE_WARNING, warn_msg, display_filename); + dialogs_show_msgbox(GTK_MESSAGE_WARNING, "%s", warn_msg); + + ui_set_statusbar(TRUE, "%s", warn_msg);
Maybe the format should be taken out of `ui_set_statusbar` and `msgwin_status_add`
`ui_set_statusbar()` has many callers using the formatting feature, and I don't think it's worth adding a specific variant not taking a format but a plain string (quick grep gives me 12 out of 87 calls pass `"%s"` using this branch). OTOH we have [`msgwin_status_add_string()`](https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea...), which I use in the patch.
or the compiler told they are [formats](https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Function-Attributes.html) [and](https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#for...).
What [do](https://docs.gtk.org/glib/func.GNUC_PRINTF.html) [you](https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea...) [mean](https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea...
@elextr commented on this pull request.
if (main_status.main_window_realized) - dialogs_show_msgbox(GTK_MESSAGE_WARNING, warn_msg, display_filename); + dialogs_show_msgbox(GTK_MESSAGE_WARNING, "%s", warn_msg); + + ui_set_statusbar(TRUE, "%s", warn_msg);
What [do](https://docs.gtk.org/glib/func.GNUC_PRINTF.html) [you](https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea...) [mean](https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea...
Oh, its only on the declaration, I looked at the [definition](https://github.com/geany/geany/blob/eef67ecc200a6cf07f277c28282facf9854cd2ea...), ok.
which I use in the patch.
which patch?
My point was that `ui_set_statusbar` printfs the input then calls `msgwin_status_add` with `%s` to re-printf the result string that it just printfed ((mis)using printf as a verb), so maybe that should use `msgwin_status_add_string()`?
12 out of 87 calls pass "%s"
Not inconsiderable. In this case you need to printf the string before sending it to the status bar because it is used in the dialog, but since status bar is always doing GTK with the result the cost of re-printfing possibly is immaterial.
@b4n commented on this pull request.
if (main_status.main_window_realized) - dialogs_show_msgbox(GTK_MESSAGE_WARNING, warn_msg, display_filename); + dialogs_show_msgbox(GTK_MESSAGE_WARNING, "%s", warn_msg); + + ui_set_statusbar(TRUE, "%s", warn_msg);
which patch?
https://github.com/geany/geany/pull/3665/commits/14d4bb0e05b85024db18349e4b8... but that's irrelevant given I didn't understand what you meant, see below.
My point was that `ui_set_statusbar` printfs the input then calls `msgwin_status_add` with `%s` to re-printf the result string that it just printfed ((mis)using printf as a verb), so maybe that should use `msgwin_status_add_string()`?
Ooh, OK. Indeed, `ui_set_statusbar()` should call `msgwin_status_add_string()` instead of `msgwin_status_add()`. I can add (pun intended) it here, although it's kind of an unrelated optimization.
12 out of 87 calls pass "%s"
Not inconsiderable. In this case you need to printf the string before sending it to the status bar because it is used in the dialog, but since status bar is always doing GTK with the result the cost of re-printfing possibly is immaterial.
Well, I can do an "optimize printf usage" pass and introduce `ui_set_statusbar_string()`, but as you say I believe it's not gonna matter at the performance level (handling format `"%s"` is likely pretty trivial, as would be handling formats without placeholders).
@techee approved this pull request.
@b4n Thanks for having a look at those! Happy Colomban, happy everyone around :-).
Apart from the minor suggestion regarding the parentheses, everything looks good to me.
@@ -906,7 +906,12 @@ static gint sort_found_tags(gconstpointer a, gconstpointer b, gpointer user_data
* followed by workspace tags, * followed by global tags */ if (t1->type & tm_tag_local_var_t && t2->type & tm_tag_local_var_t) - return info->sort_by_name ? g_strcmp0(t1->name, t2->name) : t2->line - t1->line; + { + if (info->sort_by_name) + return g_strcmp0(t1->name, t2->name); + else /* just like (t2->line - t1->line), but doesn't overflow converting to int */ + return (t2->line > t1->line) ? 1 : (t2->line < t1->line) ? -1 : 0;
I'd just suggest another pair of parentheses around the second ternary operator expression.
* { 0, 19, "geany-3.0.css" },
- */ - }; - - guint gtk_version = gtk_get_minor_version(); - for (guint i = 0; i < G_N_ELEMENTS(css_files); i++) - { - if (gtk_version >= css_files[i].min_version && - gtk_version <= css_files[i].max_version) - { - theme_fn = g_build_filename(app->datadir, css_files[i].file, NULL); - load_css_theme(theme_fn, GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); - g_free(theme_fn); - } - } -
Yeah, I kept this when removing the old GTK3 stuff thinking it could be used for possible GTK4 or future versions (ambitious plans given Scintilla doesn't support it yet) but the code can always be revived from older Geany versions so it can go.
@b4n pushed 1 commit.
974fb802a897333b003ebea568bdfb8a7194d2a2 fixup! Fix fairly nasty implicit integer conversion
@b4n commented on this pull request.
@@ -906,7 +906,12 @@ static gint sort_found_tags(gconstpointer a, gconstpointer b, gpointer user_data
* followed by workspace tags, * followed by global tags */ if (t1->type & tm_tag_local_var_t && t2->type & tm_tag_local_var_t) - return info->sort_by_name ? g_strcmp0(t1->name, t2->name) : t2->line - t1->line; + { + if (info->sort_by_name) + return g_strcmp0(t1->name, t2->name); + else /* just like (t2->line - t1->line), but doesn't overflow converting to int */ + return (t2->line > t1->line) ? 1 : (t2->line < t1->line) ? -1 : 0;
Fixed in a follow-up commit that will need to be squashed.
The other solution could be to be explicit: ```diff diff --git a/src/tagmanager/tm_workspace.c b/src/tagmanager/tm_workspace.c index 32fc36f05..6252ddd25 100644 --- a/src/tagmanager/tm_workspace.c +++ b/src/tagmanager/tm_workspace.c @@ -909,8 +909,13 @@ static gint sort_found_tags(gconstpointer a, gconstpointer b, gpointer user_data { if (info->sort_by_name) return g_strcmp0(t1->name, t2->name); - else /* just like (t2->line - t1->line), but doesn't overflow converting to int */ - return (t2->line > t1->line) ? 1 : (t2->line < t1->line) ? -1 : 0; + /* just like (t2->line - t1->line), but doesn't overflow converting to int */ + else if (t2->line > t1->line) + return 1; + else if (t2->line < t1->line) + return -1; + else + return 0; } else if (t1->type & tm_tag_local_var_t) return -1; ``` I'm not sure the verbosity is worth it though.
@b4n commented on this pull request.
* { 0, 19, "geany-3.0.css" },
- */ - }; - - guint gtk_version = gtk_get_minor_version(); - for (guint i = 0; i < G_N_ELEMENTS(css_files); i++) - { - if (gtk_version >= css_files[i].min_version && - gtk_version <= css_files[i].max_version) - { - theme_fn = g_build_filename(app->datadir, css_files[i].file, NULL); - load_css_theme(theme_fn, GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); - g_free(theme_fn); - } - } -
Yeah I figured, but if we actually do need it at some point, we can resurrect it -- or reinvent the wheel.
@b4n pushed 12 commits.
ffad343f21be323b55b3ca44c6f2d0d4d9a008fb Fix harmless GCC warning 7f630aaf917b733c76640574610017fc3a7c15ab Fix fairly nasty implicit integer conversion 30a4200bfe48fa6fd09ec66c0b1b16d0d62451ed "Fix" passing const arguments to spawn functions a8546e884d089c10cec00df593b7d2712c93a258 Add casts for irrelevant -Wwrite-strings warnings 78fe020021630cafe3838cbfbc5ed428d78790b7 Remove invalid placeholder code b0afb520fa1940d1bcf82dc4956e61aea9f4a737 Use g_*list_free_full() instead of g_*list_foreach() c37f1ae2fd74257775e8c4f9b6b75f9cb9f93626 Avoid an easy-to-fix function cast 040fc5987b2844d97e188fb8dd39e9b38743445f Silence warnings on appropriate GSourceFunc casts 8187998be80c2990b3ae49a6b654a25eb16f5346 Fix variable shadowing fe705f69ba7e2b0b22f6d02cfc12d071d4e37f3d Only pass literals as format strings for e.g. printf-style functions afe57f2ec48ff7a4b124a34f0236df54dbfb6d88 Do not use GtkResponseType as response ID type b3504ccd1b3d61587262f3993a8c3d4ac429f165 Silence some -Wcast-function-type warnings
Merged #3665 into master.
github-comments@lists.geany.org