@eht16 requested changes on this pull request.
Thanks for your updates.
It looks pretty good now beside the minor comments. I still have to test it,
will do next week probably.
> @@ -844,11 +786,13 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar
> **working_dir, guint cmd
cmd = get_build_cmd(doc, GEANY_GBG_EXEC, cmdindex, NULL);
- cmd_string_utf8 = build_replace_placeholder(doc, cmd->command);
+// cmd_string_utf8 = build_replace_placeholder(doc, cmd->command);
Would you mind removing the comment code in your changes?
> +gchar *utils_replace_placeholder(const GeanyDocument *doc, const gchar *src,
> const gchar *needles)
+{
+ GString *haystack = NULL;
+ gchar *executable = NULL;
+ gchar *replacement = NULL;
+ gint line_num = 0;
+ GRegex *regex = NULL;
+ GString *errormsg = NULL;
+
+ g_return_val_if_fail(doc != NULL , NULL);
+ g_return_val_if_fail(doc->is_valid , NULL);
+ g_return_val_if_fail(needles != NULL, NULL);
+ g_return_val_if_fail(src != NULL , NULL);
+
+ haystack = g_string_new(src);
+ if (haystack == NULL) return NULL;
While in theory correct, GLib memory allocations either succeed or the whole
application is terminated. There are basically no cases where `g_string_new()`
would return `NULL`.
> + gchar *executable = NULL;
+ gchar *replacement = NULL;
+ gint line_num = 0;
+ GRegex *regex = NULL;
+ GString *errormsg = NULL;
+
+ g_return_val_if_fail(doc != NULL , NULL);
+ g_return_val_if_fail(doc->is_valid , NULL);
+ g_return_val_if_fail(needles != NULL, NULL);
+ g_return_val_if_fail(src != NULL , NULL);
+
+ haystack = g_string_new(src);
+ if (haystack == NULL) return NULL;
+
+ /* Reduce "%%%..." to one "%" */
+ regex = g_regex_new("%+", 0, 0, NULL);
Do we need this?
It sounds good to remove accidentally double `%` but maybe the user did it on
purpose (for whatever reason).
> + }
+
+ /* if app->Project then */
+ /* all %p placeholders are already removed */
+ /* else */
+ /* fall back to absolute path/name.ext */
+ if ( g_utf8_strchr(needles, -1, 'p') != NULL ) {
+ /* replace %p with the absolute path/filename
(including extension) */
+ utils_string_replace_all(haystack, "%p",
doc->file_name);
+ }
+ }
+
+ /* Show error message if placeholders should have been replaced but are
not replaced */
+ errormsg = g_string_new("");
+
+ if ( g_utf8_strchr(needles, -1, 'd') != NULL && strstr(haystack->str,
"%d") ) g_string_append(errormsg, " %%d");
Could you wrap the code after condition to not have lines longer than 100
characters? (https://geany.org/manual/hacking.html#style)
Also, in Geany we usually do not use spaces between braces and the condition in
`ìf` statements.
> + }
+
+ /* Show error message if placeholders should have been replaced but are
not replaced */
+ errormsg = g_string_new("");
+
+ if ( g_utf8_strchr(needles, -1, 'd') != NULL && strstr(haystack->str,
"%d") ) g_string_append(errormsg, " %%d");
+ if ( g_utf8_strchr(needles, -1, 'e') != NULL && strstr(haystack->str,
"%e") ) g_string_append(errormsg, " %%e");
+ if ( g_utf8_strchr(needles, -1, 'f') != NULL && strstr(haystack->str,
"%f") ) g_string_append(errormsg, " %%f");
+ if ( g_utf8_strchr(needles, -1, 'l') != NULL && strstr(haystack->str,
"%l") ) g_string_append(errormsg, " %%l");
+ if ( g_utf8_strchr(needles, -1, 'p') != NULL && strstr(haystack->str,
"%p") ) g_string_append(errormsg, " %%p");
+
+ if (errormsg->len > 0) {
+ g_string_prepend(errormsg, "failed to substitute");
+ ui_set_statusbar(FALSE, "%s", errormsg->str);
+ }
+
I think `errormsg` is leaked here.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/4250#pullrequestreview-2745191929
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany/pull/4250/review/[email protected]>
