Please see commits for details, each commit is unrelated but I thought it best to group these small changes together to save having 3 PRs with only simple changes in. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2271
-- Commit Summary --
* build.c: Fix memory leak in prepare_run_cmd() * build.c: Refactor set_stop_button() and remove 2 unnecessary casts * build.c: Inline add_menu_accel()
-- File Changes --
M src/build.c (51)
-- Patch Links --
https://github.com/geany/geany/pull/2271.patch https://github.com/geany/geany/pull/2271.diff
ntrel commented on this pull request.
@@ -837,16 +837,17 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar **working_dir, guint cmd
run_cmd = g_strdup_printf(""%s" "%s" %d %s", helper, *working_dir, autoclose ? 1 : 0, cmd_string); g_free(helper); #else + GError *error = NULL;
This was causing an unused variable warning on Windows.
elextr commented on this pull request.
@@ -817,7 +816,8 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar **working_dir, guint cmd
{ if (vc->skip_run_script) { - utils_free_pointers(2, cmd_string_utf8, working_dir_utf8, NULL); + utils_free_pointers(3, cmd_string_utf8, working_dir_utf8, + *working_dir, NULL);
Freeing this breaks https://github.com/geany/geany/blob/a70fd97f9c841e47d4df6363c5870dcd3dd48e97... in the caller
codebrainz commented on this pull request.
@@ -1547,25 +1542,17 @@ static void set_stop_button(gboolean stop)
GtkToolButton *run_button;
run_button = GTK_TOOL_BUTTON(toolbar_get_widget_by_name("Run")); - if (run_button != NULL) - button_stock_id = gtk_tool_button_get_stock_id(run_button); + if (!run_button) return;
Minor nit, but should have space after `!` and line break before `return`.
b4n commented on this pull request.
@@ -817,7 +816,8 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar **working_dir, guint cmd
{ if (vc->skip_run_script) { - utils_free_pointers(2, cmd_string_utf8, working_dir_utf8, NULL); + utils_free_pointers(3, cmd_string_utf8, working_dir_utf8, + *working_dir, NULL);
Yeah, and as in this code path the function doesn't return `NULL` (or shouldn't, at least), it's expected that `*working_dir` contains the working directory.
I think this change is wrong. @ntrel did you actually measure a memory leak, or can this be coming from a misinterpretation of the code? (which might suggest it should be made clearer, but well)
elextr commented on this pull request.
@@ -817,7 +816,8 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar **working_dir, guint cmd
{ if (vc->skip_run_script) { - utils_free_pointers(2, cmd_string_utf8, working_dir_utf8, NULL); + utils_free_pointers(3, cmd_string_utf8, working_dir_utf8, + *working_dir, NULL);
Freed in the caller here https://github.com/geany/geany/blob/a70fd97f9c841e47d4df6363c5870dcd3dd48e97... I think.
b4n commented on this pull request.
@@ -817,7 +816,8 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar **working_dir, guint cmd
{ if (vc->skip_run_script) { - utils_free_pointers(2, cmd_string_utf8, working_dir_utf8, NULL); + utils_free_pointers(3, cmd_string_utf8, working_dir_utf8, + *working_dir, NULL);
Yes
ntrel commented on this pull request.
@@ -1547,25 +1542,17 @@ static void set_stop_button(gboolean stop)
GtkToolButton *run_button;
run_button = GTK_TOOL_BUTTON(toolbar_get_widget_by_name("Run")); - if (run_button != NULL) - button_stock_id = gtk_tool_button_get_stock_id(run_button); + if (!run_button) return;
@codebrainz Is this documented in HACKING?
codebrainz commented on this pull request.
@@ -1547,25 +1542,17 @@ static void set_stop_button(gboolean stop)
GtkToolButton *run_button;
run_button = GTK_TOOL_BUTTON(toolbar_get_widget_by_name("Run")); - if (run_button != NULL) - button_stock_id = gtk_tool_button_get_stock_id(run_button); + if (!run_button) return;
More or less: [Line break](https://github.com/geany/geany/blob/0ce5d5484e39edc25bc996cc0bf01f5bdc2b5221...), and [operator spacing](https://github.com/geany/geany/blob/0ce5d5484e39edc25bc996cc0bf01f5bdc2b5221...) (although it says two operand) but also [fit in with existing code](https://github.com/geany/geany/blob/0ce5d5484e39edc25bc996cc0bf01f5bdc2b5221...), for example [here](https://github.com/geany/geany/pull/2271/files#diff-8ae5a6e4e9368d91c0c59edb...) and [here](https://github.com/geany/geany/pull/2271/files#diff-8ae5a6e4e9368d91c0c59edb...) in nearby code (counter example [here](https://github.com/geany/geany/pull/2271/files#diff-8ae5a6e4e9368d91c0c59edb...))
ntrel commented on this pull request.
@@ -1547,25 +1542,17 @@ static void set_stop_button(gboolean stop)
GtkToolButton *run_button;
run_button = GTK_TOOL_BUTTON(toolbar_get_widget_by_name("Run")); - if (run_button != NULL) - button_stock_id = gtk_tool_button_get_stock_id(run_button); + if (!run_button) return;
I've added the line break to comply with HACKING. There are 3 existing if statements in build.c with a return clause on the same line, but I see that there are not many of these in `src/`.
space after !
I haven't done this, there are 16 matches for the regex `\bif (!\S` in build.c.
ntrel commented on this pull request.
@@ -817,7 +816,8 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar **working_dir, guint cmd
{ if (vc->skip_run_script) { - utils_free_pointers(2, cmd_string_utf8, working_dir_utf8, NULL); + utils_free_pointers(3, cmd_string_utf8, working_dir_utf8, + *working_dir, NULL);
Sorry, I messed up here. Replaced this commit with one that just fixes the unused variable.
github-comments@lists.geany.org