For instance when performing goto tag for Foo and Foo is defined as
typedef struct Foo {} Foo;
go immediately to the struct location without showing the goto popup with both the struct name and typedef.
Note the missing
g_strcmp0(second->var_type, first->name) == 0
in the check - in this particular case we won't get the type to which the typedef refers inside var_type because at the time the typedef tag is generated in c.c the struct tag doesn't exist yet. On the other hand there's no second->var_type == NULL either because this behaviour seems to be rather implementation-specific and might easily change in the future. The existing checks are probably sufficient for the real-world code. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/923
-- Commit Summary --
* Don't show the goto popup for typedef synonyms
-- File Changes --
M src/symbols.c (12)
-- Patch Links --
https://github.com/geany/geany/pull/923.patch https://github.com/geany/geany/pull/923.diff
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923
This is useful for most Geany structs like GeanyDocument which use the same name both for the struct and typedef so the jump happens immediately.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-190210010
Agreed we need something like this.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-190970752
How come it works? e.g. how comes the first tag is always the struct and the second the typedef?
Also, we probably need something like declaration/definition, if already on the stcruct and going to the same name, it's more likely one wants to go to the typedef, and that won't ever happen here if I ain't mistaken. Maybe something around the lines of "prefer a tag not on the current line" or something?
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-190971276
How come it works? e.g. how comes the first tag is always the struct and the second the typedef?
It works because tags are sorted in the order "name, line_number, ...". tm_workspace_find() will preserve this order so for
``` typedef struct Foo { } Foo; ```
we can be sure the first Foo is struct and the second is typedef. It won't work just for the single-line ones but I think these won't be very common for structs/unions/etc.
... Maybe something around the lines of "prefer a tag not on the current line" or something?
Good point, will have a look at it.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-191188630
Might be a silly question, but ...
Is this only for names that are not functions? If it includes functions how does it interact with overloaded functions where the same name occurs a number of times?
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-191189454
@elextr It's just for things that involve typedef and both the typedef and the typedef'd type have the same name.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-191190462
@b4n Done. I changed it a bit so the synonyms are removed even if the popup is shown because there's e.g. another symbol of the same name somewhere else.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192222762
Better but not great if there are more stuff of the same name in the same file, e.g. ```C
typedef struct Abc { int b, c; } Abc;
int Abc = 0;
int Abc(void); ```
It will discard the popup and always go to the struct
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192310424
@b4n Yeah, forgot to handle one more "else" case. It should be fixed now. I modified the logic slightly because I think we should also remove all tags (regardless whether they have something to do with typedefs or not) which are at the same line as the cursor - these won't cause any real goto.
By the way in your example you will never see int Abc(void); - this is a declaration, not definition.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192340962
Heh :) I'll try yours, but meanwhile I tried to sort this out on my end, and ended up with somewhat large logic changes that don't seem so absurd: d86edcaef476e16ef2e6f31b605a16893b055d36 (based off a weird branch, beware).
Tested with a ridiculously complex case: ```C typedef struct Abc { int b, c; } Abc;
extern int Abc; int Abc = 0; int Abc = 0;
int Abc(void); int Abc(void);
int Abc(void) { return 0; } ``` and the simple case: ```c typedef struct Bcd { int b, c; } Bcd;
int foo(void) { return Bcd::b; } ```
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192342461
@@ -2094,6 +2095,30 @@ static gboolean goto_tag(const gchar *name, gboolean definition) g_ptr_array_add(workspace_tags, tmtag); }
- /* If there are typedefs of e.g. a struct such as "typedef struct Foo {} Foo;",
* keep just one of the names in the list - when the cursor is on the struct* name, keep the typename, otherwise keep the struct name. Also remove tags* that are at the cursor location. */- last_tag = NULL;
- filtered_tags = g_ptr_array_new();
- foreach_ptr_array(tmtag, i, workspace_tags)
- {
if (last_tag != NULL && last_tag->file == tmtag->file &&last_tag->type != tm_tag_typedef_t && tmtag->type == tm_tag_typedef_t){if (last_tag->line == current_line)/* if cursor on struct, replace struct with the typedef */filtered_tags->pdata[filtered_tags->len-1] = tmtag;
crashes if the typedef is first, which seems to happen in my second example (simple one with `Bcd`). Just adding a `filtered_tags->len > 0` to the big conditional is enough, and I guess is the right thing to do.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923/files#r55068747
@@ -2094,6 +2095,30 @@ static gboolean goto_tag(const gchar *name, gboolean definition) g_ptr_array_add(workspace_tags, tmtag); }
- /* If there are typedefs of e.g. a struct such as "typedef struct Foo {} Foo;",
* keep just one of the names in the list - when the cursor is on the struct* name, keep the typename, otherwise keep the struct name. Also remove tags* that are at the cursor location. */- last_tag = NULL;
- filtered_tags = g_ptr_array_new();
- foreach_ptr_array(tmtag, i, workspace_tags)
- {
if (last_tag != NULL && last_tag->file == tmtag->file &&last_tag->type != tm_tag_typedef_t && tmtag->type == tm_tag_typedef_t){if (last_tag->line == current_line)/* if cursor on struct, replace struct with the typedef */filtered_tags->pdata[filtered_tags->len-1] = tmtag;
```diff diff --git a/src/symbols.c b/src/symbols.c index 56e2982..e8d6fc9 100644 --- a/src/symbols.c +++ b/src/symbols.c @@ -2104,7 +2104,8 @@ static gboolean goto_tag(const gchar *name, gboolean definition) foreach_ptr_array(tmtag, i, workspace_tags) { if (last_tag != NULL && last_tag->file == tmtag->file && - last_tag->type != tm_tag_typedef_t && tmtag->type == tm_tag_typedef_t) + last_tag->type != tm_tag_typedef_t && tmtag->type == tm_tag_typedef_t && + filtered_tags->len > 0) { if (last_tag->line == current_line) /* if cursor on struct, replace struct with the typedef */ ```
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923/files#r55068886
Okay, with the tiny fix not to crash, yours seem to work mostly well now.
By the way in your example you will never see int Abc(void); - this is a declaration, not definition.
Yeah, but we probably should do the same as the 1-tag case: if the only match was on the current line, swap definition/declaration. As we can't really use this logic with multiple tags, my attempt tried to determine which one to use depending on the current tag straight ahead. It's however a lot trickier change I'm not sure I'm comfortable with so close to the release.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192400920
@techee apparently yours breaks going to the declaration from the definition, even when there's only one match:
```c int func(void);
int func(void) { return 0; }
int main(void) { return func(); } ```
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192408589
BTW, to please @elextr here's a C++ overloaded version for testing (behaves correctly with all versions AFAICT): ```C++ class Foo { public:
int method(void); int method(int x); int method(int x, int y); int method(float x);
Foo() { method(); } };
int Foo::method() { return 0; } int Foo::method(int x) { return x; } int Foo::method(int x, int y) { return x * y; } int Foo::method(float x) { return static_cast<int>(x); } ```
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192408816
@techee I see two solutions about this: either get something working neatly and test it to death before the release, or revert the symbol popup until right after the release, which would give use some room for testing. I'm kinda in favor of reverting for now, because I don't yet have a change I feel comfortable enough 8 days before the release, but I'd be happy to be proven wrong :)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192422485
(and the current behavior is highly annoying towards structures browsing Geany's own code)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192422776
@b4n What about an imperfect (but simple) patch now and a bigger patch (possibly rewriting quite a big portion of the function) after the release? I've updated the patch so that I just removed the part of the code removing tags from the current line from the popup result (this guarantees that if there were >0 tags before the filtering, there will be >0 tags after it which is necessary for the declaration goto). The thing the patch does now is it just removes one of the two identical names one of which is typedef.
How do you feel about this? (For me the popup is one of the highlights of the next release so I'd feel a bit sorry to get it removed - but nothing tragic so I'll leave it up to you.)
Doing this properly will be indeed quite some work and definitely not for the next release. When we Ctrl+click a symbol on the current line we should decide if we want to do goto definition or declaration in case we have tags for both of them (I guess we should do the declaration jump), possibly distinguish functions based on their parameters when swapping to declaration, etc. Maybe it could be based on your patch - I didn't study it in greater detail but I like you removed the recursive call which makes things more and more complicated as the function is getting more complex.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192618283
@techee as I asked b4n on IRC, has it been tested and does it work with all other languages? Its not just C and C++ that make tags and have go to decl/defn although C++ is probably a superset of possible symbol handling there might be funnies with other languages.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192622040
@elextr This patch (in the form it is now) affects only languages having some form of "typedef". Moreover, it only affects results where the typedef has the same name as as the referenced type (e.g. struct) in a single file - I believe this is quite specific to C/C++/Objective-C where the struct namespace is separate from the global namespace so having
``` struct Foo {}; typedef int Foo; ```
is legal. For instance in the go language while it has something like typedef as well there's just one namespace for types so you can't have two types defined with the same name. I guess this will be the case of any reasonably modern language. I don't know the rest of the languages using "typedef" (Erlang, Rust, Haxe, Haskell, Verilog, VHDL) but I'd guess they won't be affected by the patch at all.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192639315
@b4n What about an imperfect (but simple) patch now and a bigger patch (possibly rewriting quite a big portion of the function) after the release? I've updated the patch so that I just removed the part of the code removing tags from the current line from the popup result (this guarantees that if there were >0 tags before the filtering, there will be >0 tags after it which is necessary for the declaration goto). The thing the patch does now is it just removes one of the two identical names one of which is typedef.
How do you feel about this? […]
Seems reasonable. I'll try it a bit but it indeed looks at least good enough for now, as it should work fine for all common cases, and only ask in weird or impossible cases.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#issuecomment-192928227
Merged #923.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/923#event-583409469
github-comments@lists.geany.org