In addition to scope, anonymous type can appear also inside var_type which should be renamed as well.
Fixes #3717. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3785
-- Commit Summary --
* Rename both scope and var_type of anonymous types
-- File Changes --
M src/tagmanager/tm_ctags.c (40) M tests/ctags/Makefile.am (1) A tests/ctags/nested_anon.c (5) A tests/ctags/nested_anon.c.tags (6) M tests/meson.build (1)
-- Patch Links --
https://github.com/geany/geany/pull/3785.patch https://github.com/geany/geany/pull/3785.diff
I tested our `glfw` build with this patch and it became perfectly reproducible + deterministic. Thanks a lot.
When will this be merged?
@b4n Does it look OK to you?
We have to rename the anonymous tag name both in scope and var_type so I moved the renaming to a separate function and call the function both on scope and var_type.
@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 :)
@techee commented on this pull request.
@@ -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);
It was really just a poor man's "improvement" of the original code which used the hard-coded value 50 (which is OK in this case because the randomized anonymous names generated by ctags are smaller). Since I moved the code to the (wannabe) generic `replace_str` function, I wanted to get rid of the hard-coded value but was lazy to do it right.
@@ -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);
OK.
+static void replace_str(gchar **where, const gchar *what, guint what_len,
+ const gchar *replacement, guint replacement_len)
Don't worry, I'm not going to use it as a base for some general-purpose string utility library ;-)
/* 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);
Right, I missed we are doing the same replacement once more below. We could maybe even use the GString version, I'm not sure if the performance is too critical here.
I checked what you propose and it looks good to me and unifies the code a little.
@@ -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;
OK.
@techee pushed 1 commit.
fb85cc0766c9e0d8fac18183fcf97add1fd87fa0 Rename both scope and var_type of anonymous types
@b4n Thanks for the review! I've just re-pushed the version with the changes you suggested.
@techee pushed 1 commit.
5196a10f764402fc3f0f55af0c9236b30df80e71 Rename both scope and var_type of anonymous types
@b4n commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
hum, the issue might be the parentheses here: as those are both unsigned, wouldn't it wrap around if `what_len < replacement_len`? if that's it, just dropping the parentheses should work (as `what_len` cannot be longer than `where_len`, and `replacement_len` cannot be negative)
@b4n commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
sorry to have created a mess in your PR…
I had to make one more change, replace `guint` with `gsize` in `what_len` and `replacement_len`: ```C static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, const gchar *replacement, gsize replacement_len) ``` because otherwise ```C gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1); ``` may underflow.
Interestingly, only meson unit tests failed and not the autotools ones - not sure why.
@techee commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
Ah, I only noticed your message now. The gsize change I made should be alright too, right?
@b4n commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
I don't see why they would? Admittedly the tests passed, but I can't see why using a larger unsigned would solve the underflow… unless maybe it subsequently overflows in final computation, putting sign back in the right direction? even if that's so, it's a bit too scary even for me :)
@b4n commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
…yeah that's what happens, so I'd rather just drop the parentheses if you're fine with it :)
@techee commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
Ah, OK, gsize isn't signed...
Anyway, I just tried to put guints back in the function parameters, dropped the parentheses, and... the `nested_anon.c` test failed. Something somewhere still isn't right.
@techee commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
Anyway, I just tried to put guints back in the function parameters, dropped the parentheses, and... the nested_anon.c test failed. Something somewhere still isn't right.
Nah, forget it, I modified the test locally, it works alright.
@techee pushed 1 commit.
aa4e901807e8f0294f25d9f4ef3516b775ce0b8d Rename both scope and var_type of anonymous types
@b4n commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
ah, good! I tested it locally and couldn't get it to fail without the parentheses, so I was starting to loose a lot of hairs over that :laughing:
@b4n commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
Sorry for the incorrect code, I actually *did* first try it using gsizes, and possibly even forgot to re-test when restoring the guints because there are no benefits to the gsizes as how the code is used (though arguably they *should* be gsize, but who cares for a couple dozen-characters strings).
Anyway, I guess that those shenanigans are the price to pay for using a fixed-size-integer language and refusing to use a higher level API because it wastes a few dozen bytes :smiling_face_with_tear:
@techee commented on this pull request.
@@ -241,6 +241,33 @@ void tm_ctags_clear_ignore_symbols(void)
}
+static gboolean replace_str(gchar **where, const gchar *what, gsize what_len, + const gchar *replacement, gsize replacement_len) +{ + if (where && *where) + { + gchar *pos = strstr(*where, what); + + if (pos) + { + gsize where_len = strlen(*where); + gchar *str = g_malloc(where_len + (replacement_len - what_len) + 1);
Yeah, sorry for your hair loss :-)
@b4n approved this pull request.
LGTM
@b4n commented on this pull request.
@@ -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);
…and hair pulling that arose from my change proved you right :)
Merged #3785 into master.
github-comments@lists.geany.org