@b4n requested changes on this pull request.
Looks good, but impl can possibly be improved a tad (see nitpicks & useful
comments inline).
Suggested changes (don't trust it blindly, esp. the part around L406):
```diff
diff --git a/src/tagmanager/tm_ctags.c b/src/tagmanager/tm_ctags.c
index ec795a31d..c7ed72fe0 100644
--- a/src/tagmanager/tm_ctags.c
+++ b/src/tagmanager/tm_ctags.c
@@ -241,26 +241,30 @@ void tm_ctags_clear_ignore_symbols(void)
}
-static void replace_str(gchar **where, const gchar *what, guint what_len,
+static gboolean replace_str(gchar **where, const gchar *what, guint what_len,
const gchar *replacement, guint replacement_len)
{
if (where && *where)
{
- guint where_len = strlen(*where);
gchar *pos = strstr(*where, what);
if (pos)
{
- gchar *str = g_malloc(where_len + replacement_len);
- guint prefix_len = pos - *where;
+ gsize where_len = strlen(*where);
+ gchar *str = g_malloc(where_len + (replacement_len -
what_len) + 1);
+ gsize prefix_len = (gsize) (pos - *where);
strncpy(str, *where, prefix_len);
strcpy(str + prefix_len, replacement);
strcpy(str + prefix_len + replacement_len, pos +
what_len);
g_free(*where);
*where = str;
+
+ return TRUE;
}
}
+
+ return FALSE;
}
@@ -399,22 +403,13 @@ static void rename_anon_tags(TMSourceFile *source_file)
{
TMTag *var_tag =
TM_TAG(source_file->tags_array->pdata[j]);
guint var_scope_len = var_tag->scope ?
strlen(var_tag->scope) : 0;
- gchar *pos;
/* Should be at the same scope level as the
anon tag */
- if (var_scope_len == scope_len &&
- var_tag->var_type && (pos =
strstr(var_tag->var_type, orig_name)))
+ if (var_scope_len != scope_len || !
var_tag->var_type ||
+ ! replace_str(&var_tag->var_type,
orig_name, orig_name_len, new_name, new_name_len))
{
- GString *str =
g_string_new(var_tag->var_type);
- gssize p = pos - var_tag->var_type;
- g_string_erase(str, p,
strlen(orig_name));
- g_string_insert(str, p, new_name);
- g_free(var_tag->var_type);
- var_tag->var_type = str->str;
- g_string_free(str, FALSE);
- }
- else
break;
+ }
j++;
}
```
> @@ -241,6 +241,29 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static void replace_str(gchar **where, const gchar *what, guint what_len,
+ const gchar *replacement, guint replacement_len)
+{
+ if (where && *where)
+ {
+ guint where_len = strlen(*where);
+ gchar *pos = strstr(*where, what);
+
+ if (pos)
+ {
+ gchar *str = g_malloc(where_len + replacement_len);
length should actually be `where_len + (replacement_len - what_len) + 1`
shouldn't it? The computation you have is simply gonna be a tiny bit wasteful
in most (all) cases, but still wrong for `what=""`.
```suggestion
gchar *str = g_malloc(where_len + (replacement_len -
what_len) + 1);
```
If we don't care about loosing a bit more space we could use GString that would
make the code a bit simpler, but it seems to have like a minimal allocation of
128 which might be a bit too much of a loss here. Unless this is a temporary
step?
> @@ -241,6 +241,29 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static void replace_str(gchar **where, const gchar *what, guint what_len,
+ const gchar *replacement, guint replacement_len)
+{
+ if (where && *where)
+ {
+ guint where_len = strlen(*where);
This should be moved inside the test below, it's only used there
> +static void replace_str(gchar **where, const gchar *what, guint what_len,
+ const gchar *replacement, guint replacement_len)
Just a remark, don't change anything for me here: this "API" is quite terrible
(not all string args have a corresponding length, and they are still not
allowed to lack a terminator anyway), but I guess it makes sense for how it's
used (saves some `strlen()`s if the anon name is used more than once).
> /* We found the parent name in the nested tag
> scope - replace it
* with the new name. Note: anonymous tag names
generated by
* ctags are unique enough that we don't have
to check for
* scope separators here. */
- if (pos)
- {
- gchar *str = g_malloc(nested_scope_len
+ 50);
- guint prefix_len = pos -
nested_tag->scope;
-
- strncpy(str, nested_tag->scope,
prefix_len);
- strcpy(str + prefix_len, new_name);
- strcpy(str + prefix_len + new_name_len,
pos + orig_name_len);
- g_free(nested_tag->scope);
- nested_tag->scope = str;
- }
+ replace_str(&nested_tag->scope, orig_name,
orig_name_len, new_name, new_name_len);
+
+ /* Do the same for var_type as well */
+ replace_str(&nested_tag->var_type, orig_name,
orig_name_len, new_name, new_name_len);
Shouldn't this new helper be used around L406?
> @@ -241,6 +241,29 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static void replace_str(gchar **where, const gchar *what, guint what_len,
+ const gchar *replacement, guint replacement_len)
+{
+ if (where && *where)
+ {
+ guint where_len = strlen(*where);
+ gchar *pos = strstr(*where, what);
+
+ if (pos)
+ {
+ gchar *str = g_malloc(where_len + replacement_len);
+ guint prefix_len = pos - *where;
conversion: pointer difference yields signed (although we know it's `>= 0` in
this case indeed), so we "need" an explicit cast
> @@ -241,6 +241,29 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static void replace_str(gchar **where, const gchar *what, guint what_len,
+ const gchar *replacement, guint replacement_len)
+{
+ if (where && *where)
+ {
+ guint where_len = strlen(*where);
Should also be `gsize` as that won't shrink `strlen()`'s return :)
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3785#pullrequestreview-2010062333
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany/pull/3785/review/[email protected]>
