Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
@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 --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="".
- gchar *str = g_malloc(where_len + replacement_len); + 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, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.