The symbol tree construction code assumes that tag parents appear before their children. At the moment tag list is sorted by line numbers which works for C structs ```C struct Foo { int bar; int baz; } ``` where the tag Foo (which is a parent of bar and baz) is sorted before bar and baz. However, this doesn't work when the name of the struct (or a variable of an anonymous struct) follows the struct members such as in the following SystemVerilog example: ```systemverilog struct packed { byte s_a, s_b, s_c; string s_str; } s_member; ``` Here the members are sorted before the parent and the symbol tree construction code doesn't work correctly.
This patch sorts symbols so that parents are sorted before their members - only if tags aren't in a parent-child relationship the original sorting by line is performed.
@b4n Does this look alright to you?
Fixes #4060. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/4063
-- Commit Summary --
* Make sure that parents appear before their children when constructing symbol tree
-- File Changes --
M src/symbols.c (40)
-- Patch Links --
https://github.com/geany/geany/pull/4063.patch https://github.com/geany/geany/pull/4063.diff
This seems to work with the example file I created (`tests/ctags/oop.sv`), and also with a small example I made. Thanks!
I found that this doesn't work if I declare the struct or typedef struct outside of a module, package, etc; but I'm not even sure that's legal in SystemVerilog. Probably not, so I wouldn't worry.
I found that this doesn't work if I declare the struct or typedef struct outside of a module, package, etc;
Could you give a short example of such code?
Could you give a short example of such code?
Never mind, I can see it too. I'll look into this.
@techee pushed 1 commit.
aa65086d6b1a29eb135680501da864038c48102b Make sure that parents appear before their children when constructing symbol tree
Should be fixed now, thanks for noticing.
Just a note about the implementation - I wanted to avoid allocation/deallocation in the compare function because of possible performance issues otherwise the code would use concatenation and be a bit simpler.
Yeah that fixed it :) Thanks!
@b4n commented on this pull request.
- if (!EMPTY(parent->scope))
+ { + if (!g_str_has_prefix(tag->scope, parent->scope)) + return FALSE; + scope_len = strlen(parent->scope); + + if (!g_str_has_prefix(tag->scope + scope_len, scope_sep)) + return FALSE; + scope_len += strlen(scope_sep); + } + + if (!g_str_has_prefix(tag->scope + scope_len, parent->name)) + return FALSE; + scope_len += strlen(parent->name); + + if (scope_len == strlen(tag->scope))
Nitpick: I'd rather write `! tag->scope[scope_len]` or one of the many equivalents that avoid yet another `strlen()`
@b4n commented on this pull request.
I have yet to test this, but it looks good and might fix those corner cases for several lesser tested languages 👍
Just note that IIUC ctags tries to avoid emitting tags in the "wrong" order to avoid these kind of issues, so it might be worth mentioning there, although I don't known if it's actually worth fixing there given others can do something similar to what you suggest here.
@techee commented on this pull request.
- if (!EMPTY(parent->scope))
+ { + if (!g_str_has_prefix(tag->scope, parent->scope)) + return FALSE; + scope_len = strlen(parent->scope); + + if (!g_str_has_prefix(tag->scope + scope_len, scope_sep)) + return FALSE; + scope_len += strlen(scope_sep); + } + + if (!g_str_has_prefix(tag->scope + scope_len, parent->name)) + return FALSE; + scope_len += strlen(parent->name); + + if (scope_len == strlen(tag->scope))
Yes, this one crossed my mind before falling asleep yesterday :-)
Just note that IIUC ctags tries to avoid emitting tags in the "wrong" order to avoid these kind of issues, so it might be worth mentioning there, although I don't known if it's actually worth fixing there given others can do something similar to what you suggest here.
Yes, but these are tags from the TM (where we sort them by name) and which were re-sorted for the symbol tree by line number. So even if ctags emits `s_member` first, this ordering is lost.
@techee pushed 1 commit.
75f6055dc09edc28ea5b2482e0c2f61fe969abed Make sure that parents appear before their children when constructing symbol tree
@b4n OK to merge this one?
I don't have a time to try it, but what I consider is better try Rust and C++ where names order and name visibility and lots of other.
github-comments@lists.geany.org