@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 :)