Well, I made the usual fatal mistake and looked around in TM and noticed there are two different implementations for getting the tag type name - I merged them in the last commit so the duplicated code gets removed.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170407348

This really should have been a separate PR, not make a huge PR bigger,

@elextr Good point, dropped the patch, will post it separately once this is merged.

and I suspect that the huge PR could have been split in the first place.

The patches are so related that it would be really hard to do. If I knew from the very beginning about all the possible problems and knew exactly what needs to be addressed, it could have been a much smaller patch set. But rewriting the patches now would be a huge amount of work.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170500045

GeanyFiletype *ft = editor->document->file_type;

• GPtrArray *tags;
• gboolean function = FALSE;
• gboolean member;
• gboolean ret = FALSE;
• const gchar *current_scope;
• const gchar *context_sep = symbols_get_context_separator(ft->id);

could use `tm_tag_context_separator()` directly

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862/files#r49339092

GeanyFiletype *ft = editor->document->file_type;

• GPtrArray *tags;
• gboolean function = FALSE;
• gboolean member;
• gboolean ret = FALSE;
• const gchar *current_scope;
• const gchar *context_sep = symbols_get_context_separator(ft->id);

Yes, it's just a little less readable IMO. It's already used twice in the code below and there's the other extra "::" I forgot so. The three occurrences justify having it assigned into a variable IMO.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862/files#r49342461

GeanyFiletype *ft = editor->document->file_type;

• GPtrArray *tags;
• gboolean function = FALSE;
• gboolean member;
• gboolean ret = FALSE;
• const gchar *current_scope;
• const gchar *context_sep = symbols_get_context_separator(ft->id);

No no, I just meant calling `tm_tag_context_separator(ft->lang)` here instead of `symbols_get_context_separator(ft->id)` to avoid the indirection of `symbols_get_context_separator()` itself calling `tm_tag_context_separator()`.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862/files#r49343236

Attached are the diffs for v2 and v3 - the differences are quite nicely visible when comparing the two in a diff viewer […]

It's easy to simply `git diff techee/tm_workspace_find_cleanup2 techee/tm_workspace_find_cleanup3` :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170649945

-/* Returns all matching members tags found in given struct/union/class name.

• @param name Name of the struct/union/class.
• @param file_tags A GPtrArray of edited file TMTag pointers (for search speedup, can be NULL).
• @return A GPtrArray of TMTag pointers to struct/union/class members */

-const GPtrArray * -tm_workspace_find_scope_members (const GPtrArray * file_tags, const char *name,

• 						 gboolean search_global, gboolean no_definitions)
  

+/* Returns tags with the specified prefix sorted by name. If there are several

• tags with the same name, only one of them appears in the resulting array.
• @param prefix The prefix of the tag to find.
• @param lang Specifies the language(see the table in parsers.h) of the tags to be found,

•         -1 for all.
  

• @param max_num The maximum number of tags to return.
• @return Array of matching tags sorted by their name.

+*/ +GPtrArray *tm_workspace_find_prefix(const char *prefix, langType lang, gint max_num)

OK.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862/files#r49366909

I'm sure we discussed it but I can't find it back quickly apparently; but there's something odd I believe should work: currently nothing is suggested for namespaces. E.g., imagine this C++ file: ```c++ namespace Ns { class Cls { public: static int f() { return 42; } }; };

int main(void) { return Ns::Cls::f(); } ``` Nothing pops after `Ns::`, but it works for `Cls::`.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170667262

}

• else if (typed != '.')

• return;
  

• if (typed == '.')

• pos -= 1;
  

• else if (match_last_chars(sci, pos, context_sep))

• pos -= strlen(context_sep);
  

• else if ((ft->id == GEANY_FILETYPES_C || ft->id == GEANY_FILETYPES_CPP) &&

• 	match_last_chars(sci, pos, "->"))
  

• pos -= 2;
  

• else if (ft->id == GEANY_FILETYPES_CPP && match_last_chars(sci, pos, "->*"))

• pos -= 3;
  

• else

For PHP and Zephir (if we get completion for namespaces, see the comment in the PR itself) it would be good to support `` separator which is used to separate namespaces (`Namespace1\Namespace2\Class::member` -- yes, PHP is weird).

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862/files#r49374973

}

• else if (typed != '.')

• return;
  

• if (typed == '.')

• pos -= 1;
  

• else if (match_last_chars(sci, pos, context_sep))

• pos -= strlen(context_sep);
  

• else if ((ft->id == GEANY_FILETYPES_C || ft->id == GEANY_FILETYPES_CPP) &&

• 	match_last_chars(sci, pos, "->"))
  

• pos -= 2;
  

• else if (ft->id == GEANY_FILETYPES_CPP && match_last_chars(sci, pos, "->*"))

• pos -= 3;
  

• else

Well, unless the \ is needed for something else, I'd suggest "normalizing" the scope upon receiving the tag from the ctags parser by replacing the \ with :: so we don't have to care later.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862/files#r49388189

The thing is that scope completion shows tags only from a single file (so

e.g. only members of a single anon_struct_0 are shown). Namespaces can span multiple files and then you'd get members from just a single (more or less random) file which might look strange. So I skipped namespaces.

That is likely to make autocompletion worse than it is now. Everything in C++ is namespaced (or should be) so what you are saying is that no name in a namespace will provide an autocompletion.

C++ autocompletion is such rubbish that I now turn it off. Python autoc is such an annoyance I turn it off. Basically I turn just it off completely. And decisions to arbitrarily not support large parts of a languages features doesn't help.

Thank you for putting the effort into it, but instead of using your effort trying to solve a broken system it would be a better spent to provide the hooks so plugins can control these things, with libclang being to obvious contender for C++.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170723699

On 12 January 2016 at 09:59, Jiří Techet notifications@github.com wrote:

That is likely to make autocompletion worse than it is now.

How? This definitely didn't work before. What about trying it first?

​Things in namespaces autocomplete now (I turned it on to check :).​

Everything in C++ is namespaced (or should be) so what you are saying is that no name in a namespace will provide an autocompletion.

It doesn't work for namespaces themselves only but works for the stuff inside - have a look at Colomban's example - you get scope completion for the contents of the classes inside.

​Yes, Colombans example makes it clear that its has gone backward since namespaces work now.​

C++ autocompletion is such rubbish that I now turn it off. Python autoc is such an annoyance I turn it off. Basically I turn just it off completely. And decisions to arbitrarily not support large parts of a languages features doesn't help.

I can only say - patches are welcome. I'm not saying it's perfect but it's much better than before (which was really broken in some cases) and can always be improved. I don't want to put more and more patches on top of this (as you suggested yourself yesterday) because then it never gets merged.

​Sure, but saying "I have created a regression by ignoring namespaces but I will fix that in another PR very soon" is very different to ​saying you have created a regression and won't fix it.

Thank you for putting the effort into it, but instead of using your effort trying to solve a broken system it would be a better spent to provide the hooks so plugins can control these things, with libclang being to obvious contender for C++.

This is something I'm not interested in - if you want to make clang work then clang will have to have full knowledge of the build process which means Geany will have to start managing your project. And this is not something I want Geany to evlove into - I want Geany to be build system independent and cross-language. If I wanted something like this, I'd just grab some full-blown IDE - I feel zero motivation to recreate something like that. And making such a thing work is A LOT more effort than the few hundreds lines in this patch.

​Indeed, but the more effort that goes into the bullt-in system the harder it becomes to allow plugins to replace it.​ Matthew had a look at libclang, but found that it was not possible to inject the information libclang produces into Geany IIRC. If a plugin wants to be a "full blown" IDE it should be allowed to be (but I won't be doing it any time soon).

I definitely wouldn't call the ctags parsing stuff "broken" - actually think the ctags parsing works wonderfully well (and is super-fast). It just depends what expectations you have - it won't be able to do some things like things that require function body parsing.

​If it gives lots of wrong options and misses lots of right options its broken. ​ It may be broken due to limitations imposed for good reason (as you say above) but its still broken.

To be honest, I also don't care much about the autocompletion but the goto tag definition/declaration feature is fantastic (and getting #406 https://github.com/geany/geany/pull/406 merged would make a much bigger practical difference for me than these patches).

​That indeed looks like a good idea, pinged it for you :)​

The main benefit of these patches is mainly the TM cleanup - the patches merged a year ago concentrated mostly on tag sorting and management, these patches clean up searching. Some of the previous code was *really* bad and hard to read and I think this will make TM much easier to maintain. The scope completion improvements are more or less a side-effect of this. There is some more TM-related work I'd like to do (Colomban, don't worry, should be more or less formal cleanups and not such headache-makers like this :-) but I think the code is pretty understandable now.

​That is a good reason to make the changes, but it didn't really come across that way. But again, code cleanup at the cost of regressions isn't good.

— Reply to this email directly or view it on GitHub https://github.com/geany/geany/pull/862#issuecomment-170737058.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170742045

On 12 January 2016 at 12:08, Matthew Brush notifications@github.com wrote:

Things in namespaces autocomplete now (I turned it on to check :).

Doesn't work here. Maybe you misunderstood what @b4n https://github.com/b4n was saying? Here's an example:

namespace Foo { void bar() {} }int main() { Foo::<<<<---- no completion suggestions here }

​Ok, yeah I misunderstood what he was talking about.​ I understood it to mean the symbols in the namespace would not be available at all, ie if I had typed `Foo::ba` then "bar" would not be available for completion.

The current way (and likely after this PR too) is particularly broken in that it gives auto-completions for stuff that is in an inaccessible namespace. For example:

namespace Foo { void blah_blah() {} }int main() { blah_bl<<<<---- gives an invalid completion for "blah_blah" here }

​Which is the sort of thing that gets it turned off :)​

Personally I don't use auto-completion to speed up typing, I use it for exploring symbols I can validly use at that point in the code and to get them spelt correctly, and the current case-sensitive, prefix-matched, kitchen sink auto-completions causes me to use IDE with "smart" auto-completions when I'm writing C++ (ex. QtCreator, VisualStudio).

​Though that has its problems too :(​

— Reply to this email directly or view it on GitHub https://github.com/geany/geany/pull/862#issuecomment-170759070.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170764606

@elextr Good. I'm not aware of any regressions compared to the previous behaviour and definitely don't want to introduce any so if you run into something that worked before and doesn't now, just let me know.

@codebrainz

Personally I don't use auto-completion to speed up typing, I use it for

exploring symbols I can validly use at that point in the code and to get them spelt correctly

Yeah, exactly how I use the normal autocompletion too :-). The scope completion should show better results, but unfortunately it will be triggered quite rarely.

and the current case-sensitive, prefix-matched,

kitchen sink auto-completions causes me to use IDE with "smart" auto-completions when I'm writing C++ (ex. QtCreator, VisualStudio).

I think it would be possible to sort the tags in a case-insensitive way and make the completion case-insensitive. Shouldn't be that hard. On the other hand because we are searching in all tags, not doing the prefix search might get too slow.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-170896024

The thing is that scope completion shows tags only from a single file (so e.g. only members of a single anon_struct_0 are shown). Namespaces can span multiple files and then you'd get members from just a single (more or less random) file which might look strange. So I skipped namespaces.

It probably is fixable but it would probably be a little more work so I'd skip it for now.

Okay, get it. Maybe in the end it'd require a special case to allow results for namespaces to span multiple files or something. But ok, we can leave it like this for now and keep further improvements for later too :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-171365062

Okay, get it.

Maybe in the end it'd require a special case to allow results for namespaces to span multiple files or something. But ok, we can leave it like this for now and keep further improvements for later too :)

Yeah, the special case will have to start in tm_workspace_find_scope_members() already so we skip the "search in circles" which would start by a single file and we might get results just from it. And there should be some optimization because in theory it might require going through all tags. But I think it's doable.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-171459667

@techee yes (at least for the moment, you never know when I find something else to fix :])

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-171462638

I somewhat am, for the 3 small details I mentioned and you ACKd, but I'm also testing it a little on my end before merging it (looking great so far).

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-171475447

@techee it's fine by me, the changes are small, I trust you and I can easily diff the changes. So please, go ahead :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-171639988

@techee great :) LGTM for merge now, I'm just testing it a bit longer but should be GTG.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172022466

@b4n Just noticed a minor problem (or at least a strange looking thing). If you open for instance tm_tag.h and look at the symbol tree in the sidebar, the Typedefs/Enums are combined and because the anonymous enums get the name of the typedef now, the entries under Typedefs/Enums are twice there - once as enum and once as typedef.

One possibility to make it look less strange would be separating typedefs and enums into different roots (grouping them under the same root is a bit artificial anyway since they don't have much in common). But I think all existing roots are already occupied for C/C++ so we'd have to create a new one. What do you think?

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172126077

Alright, while at it I added also the separate root for enums for the sidebar I suggested. I don't want to delay the merge of the patch set so just tell me and I can make a pull request separately for the last two patches.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172237383

By the way I also have the namespace completion patch - it was pretty straightforward. Can add it here or post separately.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172344577

By the way I also have the namespace completion patch - it was pretty straightforward. Can add it here or post separately.

Great! But hold it a little and make a separate PR when this one is merged, that'll be simpler for me I think :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172587760

I've just repushed the patches with the following changes:

1. removed the anon_* tag sidebar renaming patch based on the corresponding typedef. 2. removed the patch removing the anon tag number in the sidebar. 3. Removed the patch adding the new root for enums in sidebar (will post separately). 4. Used tm_tags_dedup() to remove duplicates in scoped search.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172617101

Hum, it's odd, 1281d0c942322d99da626ad00edd6077425f2c8d and 04a200b34b4c7415114122e9be01bdcae6c7e7b1 have the same commit message. Overall diff looks good though

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172659686

Scope completion doesn't seem to like not being called at the root scope or on a scoped type: ```c++ namespace ns { class C { public: void func() {} int memb; }; C c; void dummy() { c.func(); // no autoc on c. } };

ns::C c2;

int main (void) { return c2.memb; // neither on c2. } ``` (Same if trying to autoc outside a func). Any reason? it's not limited to namespaces, same happens with e.g. a class.

----

However, I must say this is beautiful: ```c++ class C;

class A { public: C *bar; };

class B { public: A foo; };

class C { public: B *func() { return &b; } int memb; B b; };

C c;

int main (void) { // scope completion all the way, with only sensible stuff :) return c.b.foo.bar->b.foo.bar->b.foo.bar->func()->foo.bar->memb; } ```

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172661976

@b4n Fixed the first problem - see the new patch. The ns::C c2 problem is however a parser problem and in fact I believe the same as Lex reported here:

https://github.com/geany/geany/issues/845

In your example the var_type of the "c2" TMTag is "ns" instead of "ns::C".

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172683720

Damn, you're right. Though, while trying to fix this in the C parser, I don't think it works now it reports ns::C. Probably the type is not split up on scope separator, so a qualified type doesn't work I guess?

Right. In this case I think we want the scope to be part of var_type because we need to distinguish

``` class Foo { Bar bar; } ```

from

``` Foo::Bar bar; ```

I think the type-scope splitting of var_type could be done in the scope completion code when the parser gets fixed. But even after that it won't do any magic - we for instance don't see things like "using namespace Foo;" so scope completion won't work in all cases.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-172791492

Eh, forgot about markdown - the end of the sentence that doesn't make sense is

``` ...and also between [ and ] (stripping arrays). ```

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-175079205

OK. I guess having const, volatile and alike removed would be good too, but that should probably be rather parsed as var type attributes by the parser itself… but well.

I have a (maybe naive) idea about a more or less complete sync of our ctags with universal-ctags. Then we should start thinking about what extra tagEntryInfo entries we need to port to universal-ctags. To make the extra tag entries acceptable for universal-ctags, they should be as generic as possible. So for instance IMO var_type makes sense as an additional entry in the tags file of universal-ctags but we shouldn't introduce some entries too specific to our use of ctags that wouldn't make sense for anything else than Geany. If we need something too TM-specific, we should adjust the TMTag based on our needs inside TM and not ctags.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-175089970

Exactly, I meant to leave c.c at the very end of the syncing process - if ever. Another problem is the use of Gregex in Geany - I believe it replaced GNU regex for performance reasons on Windows but it would be worth re-evaluating if it's still needed (maybe msys2 uses a newer version which doesn't suffer from the performance problem or maybe the regexes themselves can be optimized to avoid some backtracking if this was something that caused the poor performance).

Another topic is MIO which I think might be interesting for universal-ctags because 1. The performance is better when the source file is first loaded to MIO and then parsed. 2. If they want to have some kind of ctags library, they'll need something like that anyway.

---

As the first step I'd like to separate the parser sources from the "main" sources like in universal-ctags so it's easier to diff. When moving stuff around, I was thinking it might be a good idea to move the TM itself (the tm_* files) inside the Geany src directory (e.g. under a "tm" subdirectory) - it's really Geany source now - there'll never be any upstream and it would be possible to use Geany's utility macros to make the sources a bit more Geany-like. Then the whole "tagmanager" directory could be renamed to ctags and it would only contain the stuff that needs to be synced with universal-ctags. What would you think about such TM source reorganization?

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-175099418

OK, I'm using this for some time now and all seem just great :) Thanks a lot!

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/862#issuecomment-182888660