This is rebased #3175 with cleaned up commit history but no functional change. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3185
-- Commit Summary --
* Rename tm_tag_file_t to tm_tag_local_var_t * Enable local tag generation for C/C++ * Update goto symbol definitions to take into account local variables * Update (non-scope) autocompletion to take into account local variables * Update scope completion to take into account local variables * Strip more things from variable type * Support (multiple) inheritance by scope completion * Sort tags so tags from source file's header are preferred to other files * Rewrite member_at_method_scope() to handle more situations * Sort non-scope autocompletion results by locality * Make validity of local variables configurable based on used language
-- File Changes --
M src/editor.c (15) M src/symbols.c (13) M src/tagmanager/tm_parser.c (26) M src/tagmanager/tm_parser.h (4) M src/tagmanager/tm_workspace.c (546) M src/tagmanager/tm_workspace.h (6)
-- Patch Links --
https://github.com/geany/geany/pull/3185.patch https://github.com/geany/geany/pull/3185.diff
@techee pushed 1 commit.
bfd38b6a1ff509ab3df7222caceb0978fa52ecd6 Fix obtaining inherited class members when defined in other file
What the heck, why a new PR? So much invaluable discussion in the old PR that won't be directly connected.
What the heck, why a new PR? So much invaluable discussion in the old PR that won't be directly connected.
See the end of the discussion - I wanted to rebase the PR because there were many fixups and reverts and other mess. If I rebased it there, it wouldn't be clear what the discussion was about because the commits wouldn't correspond to the discussion. The PR is linked at the top of this PR so I don't see any problem.
Tested in the same place as before, and some other places, works as expected.
This is such a big improvement on previous capabilities it should be merged and improvements can be added later. If there are no changes and no objections I will merge as is after the weekend.
Then hopefully it gets some testing and any actual performance issues can be identified rather than more premature optimisation.
(The idea is to merge before you think of something else and mess this PR up too :stuck_out_tongue_winking_eye: )
(The idea is to merge before you think of something else and mess this PR up too 😜 )
Good strategy :-).
But I'm pretty happy with the result after testing it a little more (and discovering the easy-to-fix problems with inheritance from other files). So yeah, I agree.
Maybe #3176 could be merged too at the same time - I think it's a low-risk and simple patch, also related to autocompletion and hopefully not very controversial.
I approved #3176 a while ago, question is why it's not merged already :-)
@kugel- requested changes on this pull request.
I'm not really happy with b710cf3aa1300dd43e4edfcf152f7f3a10f9a686
- it makes a lot of assumption - it seems really expensive - works only for C/C++ by hardcoding - has nothing to do with local variables
Please lets discuss that feature in a separate PR. Also, if we want that, then it may make sense to expose that "find header" logic somehow to implement a "go to header" feature (I think ProjectOrganizer already has something like that?)
foreach_ptr_array(tmtag, i, tags)
{ + if (tmtag->type & tm_tag_local_var_t && + (doc->tm_file != tmtag->file || + current_line < tmtag->line ||
`tm_parser_var_valid_before_declare()` should be called here, no?
Also, the whole condition is basically a duplicate of the same in `is_valid_autocomplete_tag()`. Can `is_valid_autocomplete_tag()` be made callable from here (perhaps with a different name)?
@@ -601,6 +601,7 @@ static void show_autocomplete(ScintillaObject *sci, gsize rootlen, GString *word
} /* store whether a calltip is showing, so we can reshow it after autocompletion */ calltip.set = (gboolean) SSM(sci, SCI_CALLTIPACTIVE, 0, 0); + SSM(sci, SCI_AUTOCSETORDER, SC_ORDER_CUSTOM, 0);
I read the docs but I couldn't grasp the difference between `SC_ORDER_PRESORTED` and `SC_ORDER_CUSTOM`. In either case scintilla does not sort by itself. `SC_ORDER_PRESORTED` seems to assume the list is presorted alphabetically but what's the deal with that?
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Every predicate function contains the is_autocomplete_tag() check. I think it makes sense to have that call here (before calling the predicate). The function could be suitably named `copy_autocomplete_tags()`
That should perform a bit better
@techee commented on this pull request.
foreach_ptr_array(tmtag, i, tags)
{ + if (tmtag->type & tm_tag_local_var_t && + (doc->tm_file != tmtag->file || + current_line < tmtag->line ||
tm_parser_var_valid_before_declare() should be called here, no?
In fact, I'm less convinced whether `tm_parser_var_valid_before_declare()` is a good idea now - even though you can declare (i.e. assign) variables after their first appearance in file, I think this pattern is not so common and like in the C/C++ case because of this you will get invalid variables in the autocompletion in many cases which IMO is not worth it.
Also, the whole condition is basically a duplicate of the same in is_valid_autocomplete_tag(). Can is_valid_autocomplete_tag() be made callable from here (perhaps with a different name)?
I was thinking about it but there's small difference - while with autocompletion we have only prefix, here we have the whole symbol name so less "false positives" in the result so I thought I could leave it this way. In any case, I don't have a strong opinion about this and these two could indeed be unified. This function could go to tm_tag.c.
@techee commented on this pull request.
@@ -601,6 +601,7 @@ static void show_autocomplete(ScintillaObject *sci, gsize rootlen, GString *word
} /* store whether a calltip is showing, so we can reshow it after autocompletion */ calltip.set = (gboolean) SSM(sci, SCI_CALLTIPACTIVE, 0, 0); + SSM(sci, SCI_AUTOCSETORDER, SC_ORDER_CUSTOM, 0);
I didn't explore the logic of how we show the popup and if we need it (but @elextr seemed to experience some problem without it). The way I understand it is that when we call `SCI_AUTOCSHOW`, Scintilla takes care of filtering the list based on what the user types unless we update it ourselves. When the list is sorted, Scintilla can bisect where the start/end of the range to be displayed is and show the values in between (this really is my assumption, I haven't checked Scintilla sources). When not sorted, it first creates an index that corresponds to alphabetic ordering and does the bisection over this index. This makes the complexity `2*log(N)` (plus the initial creation of the index) instead of `N` if it had to go through all the values every time.
@techee commented on this pull request.
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Can do, I would just move it after `predicate()` - `is_autocomplete_tag()` potentially makes string comparison of scopes and it's better to eliminate those with the other conditions first.
@kugel- commented on this pull request.
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Hm ok. I was more concerned about calling a function via pointer and hoped it could be avoided. But a string comparison isn't negligible either. And I don't really want to start doing micro-benchmarks here.
@kugel- commented on this pull request.
@@ -601,6 +601,7 @@ static void show_autocomplete(ScintillaObject *sci, gsize rootlen, GString *word
} /* store whether a calltip is showing, so we can reshow it after autocompletion */ calltip.set = (gboolean) SSM(sci, SCI_CALLTIPACTIVE, 0, 0); + SSM(sci, SCI_AUTOCSETORDER, SC_ORDER_CUSTOM, 0);
thanks
I'm not really happy with https://github.com/geany/geany/commit/b710cf3aa1300dd43e4edfcf152f7f3a10f9a6...
it makes a lot of assumption
Basically the only assumption that this makes is that sources and corresponding headers have the same file name and headers have some of the commonly used header extensions.
it seems really expensive
Note that this doesn't use any file system operations - this works over TMSourceFile structures which represent parsed source files, which, without any extensions, are only open documents (with something like ProjectOrganizer it can be whole projects). It's true this function does string comparisons which can be a bit expensive but the number of files tends to be 2 orders of magnitude less than the number of tags.
works only for C/C++ by hardcoding
This is true but I don't see any sign that any of the other ctags parsers would get the capabilities of the cxx parser soon so scope autocompletion will unfortunately be C/C++ only feature for the foreseeable future.
has nothing to do with local variables
It doesn't but neither other things in this PR do - this is about improving scope autocompletion. When you type something like ``` void MyClass::myFunction() { name. // <-- } ``` it makes a difference whether the code first looks for a member `name` in the header or an arbitrary member `name` in the workspace - in which case you get incorrect results.
In fact, I was thinking about improving this by looking at `#include` tags (we currently ignore them) and using tags from the included files first.
Now thinking about it I should have implemented it differently - I could do it inside `sort_found_tags()` and compare the file name of the sorted tag with the source file name there Will be less comparisons and shorter code.
Please lets discuss that feature in a separate PR.
I can separate it but in some form I want to keep this feature because it improves the results of scope autocompletion.
Also, if we want that, then it may make sense to expose that "find header" logic somehow to implement a "go to header" feature (I think ProjectOrganizer already has something like that?)
The difference between ProjectOrganizer and Geany is that Geany doesn't know about all project files (there's no notion of a file belonging to a project in Geany). For every project file in ProjectOrganizer there is TMSourceFile, in Geany just for open files and this functionality cannot be easily replicated in Geany itself unless Geany would search the file system for the header.
I approved https://github.com/geany/geany/pull/3176 a while ago, question is why it's not merged already :-)
I wasn't sure whether LGBI is sufficient approval ;-). Merged now.
Yea, a lot of changes that don't relate to local tags, true. Ideally these would all be separate PRs. It makes reviewing the necessary changes for local tags harder than necessary.
However, the "non-header" changes are less invasive contentious from what I saw so I'm willing to accept them as part of this PR.
With ProjectOrganizer there can be lots of open files so I'm a bit concert about simply iterating through all of them. Also, there might be header files with the same name, so some kind of clever resolution is needed. Anyway, the patch for that feature is large enough to warrant a separate PR.
it makes a difference whether the code first looks for a member name in the header or an arbitrary member name in the workspace - in which case you get incorrect results.
Self correction - scope completion will probably find the right class (though looking at tags from headers definitely won't hurt). But it's definitely useful for non-scope autocompletion.
@elextr commented on this pull request.
@@ -601,6 +601,7 @@ static void show_autocomplete(ScintillaObject *sci, gsize rootlen, GString *word
} /* store whether a calltip is showing, so we can reshow it after autocompletion */ calltip.set = (gboolean) SSM(sci, SCI_CALLTIPACTIVE, 0, 0); + SSM(sci, SCI_AUTOCSETORDER, SC_ORDER_CUSTOM, 0);
A quick scan of the code suggests Scintilla makes the index in all cases, but only sorts it in the `SC_ORDER_CUSTOM` case, so the sort is the extra cost. Then as @techee said it binary searches the index for the selection each time you type, so when we passed `SC_ORDER_PRESORTED` by leaving the default it was binary searching an unsorted index and that gives undefined results, which is probably why @techee didn't see it and I did.
@elextr commented on this pull request.
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Yeah, lets leave it and start worrying if it ever actually causes an issue.
@kugel- are you explicitly blocking this until @techee splits it into two PRs? Or now you have reviewed it any way is it ok this time but don't do it again?
Please separate PRs
@kugel- why do you come in at the last minute and require a PR to be split wasting the effort others have done to test and check it?
Its not like this project has masses of spare resources, and making it harder is not the way to get improvements included.
@kugel-
Ideally these would all be separate PRs. It makes reviewing the necessary changes for local tags harder than necessary.
I'm willing to accept them as part of this PR.
Why have you changed? My question related to your review and if I should dismiss it.
I can't follow. Yesterday I tried to review the whole thing for the first time. I was striked how large the change has become since my initial, very coarse, look (that wasnt a review) and have found that a lot of unrelated changes (w.r.t. to local tags) were added. GitHub even hides the diff for tm_workspace by default!! Since I also want this feature I'm trying to accept most of them in one PR although I don't feel truly confident. But the header feature a bit too much for my taste and should be dealt with separately.
From what I can see its a single commit that has to be backed out, provably not much of a hassle.
For the future I wish that PRs stay focused on what they were created for and don't accumulate many unrelated changes.
@techee commented on this pull request.
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Call over pointer doesn't matter at all, it's still the same function in the loop and the processor will handle it for free. Having to compare scope for local variables is another story - with local variables enabled we get 3x more tags than before and eliminating the comparison early by checking whether we are in the right file definitely helps.
@kugel- commented on this pull request.
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Regardless of the order, IMO the call to `is_autocomplete_tag()` doesn't need to be duplicated in each predicate function but can be called in one place here, especially if the compiler whats to inline it.
Indirect calls aren't as free as direct calls but it lets not focus on that, I don't care much.
I can't follow. Yesterday I tried to review the whole thing for the first time. I was striked how large the change has become since my initial, very coarse, look (that wasnt a review) and have found that a lot of unrelated changes (w.r.t. to local tags) were added. GitHub even hides the diff for tm_workspace by default!! Since I also want this feature I'm trying to accept most of them in one PR although I don't feel truly confident. But the header feature a bit too much for my taste and should be dealt with separately.
For me personally, the problem is your attitude. You didn't indicate in #3175 that you are going to spend some more time with this PR and the conversation was mostly between me and @elextr. From the beginning of this PR you started bitching about "why a new PR", what you are "willing to accept" and about (in your opinion) "unrelated changes" that were introduced to the PR. The effect on me is significant reduction of willingness "to obey".
Those "unrelated changes" as you call them come from the testing of how autocompletion works with local variables enabled. It offers many more results than before and it's not sufficient to just simply enable local variables. I spent quite a bit of time testing how autocompletion works and the patches on the way are the result of the problems I (or @elextr ) run into (like the sorting of non-scope autocompletion tags was a very good idea Lex had and having the tags from the header means that in C++ you have members of classes - which you typically access frequently - at the top of the list). I normally try to separate patches as much as possible but at the same time I wanted to make the result useful and not annoying because you will see scope completion much more frequently and you will see more symbols in non-scope autocompletion. No need to remind me about making compact PRs, I try to do that with truly unrelated code but this isn't the case.
In any case, right now I suggest not to merge this PR until all problems are resolved and everything is discussed - I think there's no rush. You mention you "don't feel truly confident" so please take your time and post the things that are questionable in your opinion here. At the same time, I'd like to have the discussion about the header patch here too - I'm not going to spend time to remove it just because you happen not to like it - I'd like to see some real arguments (I tried to reply to your initial comments) and know what you suggest instead.
As I mentioned, I'd like to generalize it so symbols from included files are also sorted towards the top of the list - this would probably mean to change the implementation of the function checking whether the file is a header (or include) by having a hash table (simulating a set) with the included file names, going through TMSourceFile's and checking whether the file name is in the table.
The "look for files that could be headers corresponding to this C file" is clearly a new feature that has *nothing* to do with local tags. I did not realize that this feature was added to the previous PR until starting an in-depth review on this PR. I also never said that I don't like it or do not want it in Geany, I just want to discuss and review it separately because I have some doubts about the implemented assumptions and I don't want the "local tags" effort to be blocked by that. It's also a big chunk of the overall diff and I like to review smaller diffs.
I followed your discussion on the other PR but I simply didn't have the time for a in-depth review yet. Sorry for being late to the party.
If you're not going to spend time to remove it then I won't spend any more time to review this PR. Clearly you have @elextr already be happy enough to get this over the finish line without my involvement - and that's perfectly fine. There is also nothing in the diff that's fatally wrong so just go ahead.
@kugel- commented on this pull request.
(github wants me to comment)
@kugel- I don't see the searching and sorting and header parts as something separate from enabling local variables, they are an integral part of presenting the locals to the user in a usable manner.
You complained about separation of the conversation on #3175 but it does not appear you understood it. Otherwise you would have understood that without the filtering and the ordering parts of the PR unrelated globals from the C/C++ global tags files overwhelm the locals in the autocomplete list and make them unusable.
And without the work for selecting the most likely base symbol for scope autocomplete it is also less useful since it will be more likely to present completions for the wrong variable.
Locals are inherently scoped, and inner scopes hide outer scopes. So "enabling locals" inherently involves handling scopes, and those rules are inherently language specific, see the examples in #3175. Geany does not have exact scope detection, especially with include files, so the sorting and filtering, including (for C/C++) prioritising header file symbols over globals provides a heuristic for presenting the most likely candidates at the top of the autocomplete list, and choosing the most likely base for scope autocomplete so making them usable
Splitting the PR now means more work, double testing, the initial PR presenting a bad experience, and the risk of introducing bugs in the split.
For all the above reasons I do not support splitting this PR.
I did react negatively to your blocking and seeming to demand it be split after seeming to accept the PR as I pointed out above, and I apologise for that. Perhaps you didn't mean to seem to accept it, and you didn't mean to be demanding without reason. English is imprecise and usages vary around the world, here "please do x" means "I would like you to do x if its reasonable", whereas "do x please" means "I demand you do x and am saying please just to be polite", but I am not sure its the same in other English speaking countries, let alone when English is not a first language.
@kugel- didn't see your latest comment before posting the above, thank you.
I do want to get this in master so it gets tested more, simply because its user experience depends on a heuristic and there may be edge cases that @techee and I have missed where it fails.
@kugel- All I want is to have the header stuff discussed **now** so I know what you think is wrong about it. It means some extra work for me and if it turns out that the discussion evolves into "let's leave it the way it is" then the work is wasted. This is why I'm not really thrilled about comments like "Please separate PRs" without proper discussion about what you think is wrong, what you'd suggest to change or whether you want to scrap the feature altogether. I'm totally fine to rework the patch, separate it to a new PR or scrap it if it turns out not to be a good idea but **after** a proper discussion and not just because someone orders me to do so.
If your primary concern is performance, I suggest to change the implementation so we add a hash table `file_name -> TMSourceFile` to `TMWorkspace` https://github.com/geany/geany/blob/55aea4cb243decf288b7ae0c397a120d6c5b806a... so we have a fast access to `TMSourceFile` based on file name. Then we can perform a fast lookup of a header (constructing possible header candidates using common C/C++ header extensions and looking them up in the hash table - that is 6 lookups for the headers I used which is super-cheap) and this could also serve for included file lookups if we start using the include tags.
@techee commented on this pull request.
!(tag->type & tm_tag_local_var_t) &&
+ is_autocomplete_tag(tag, info); +} + + +static guint copy_tags(GPtrArray *dst, TMTag **src, guint src_len, GHashTable *name_table, + gint num, gboolean (*predicate) (TMTag *, CopyInfo *), CopyInfo *info) +{ + guint i; + + g_return_val_if_fail(src && dst, 0); + + for (i = 0; i < src_len && num > 0; i++) + { + TMTag *tag = *src; + if (predicate(tag, info) &&
Regardless of the order, IMO the call to is_autocomplete_tag() doesn't need to be duplicated in each predicate function but can be called in one place here, especially if the compiler whats to inline it.
Agree.
And if performance is your concern, we can actually do better. I didn't think of it when writing the patch but these calls ``` copy_tags(dst, found, count, name_table, max_num - dst->len, is_local_tag, info); copy_tags(dst, found, count, name_table, max_num - dst->len, is_current_file_tag, info); copy_tags(dst, found, count, name_table, max_num - dst->len, is_header_file_tag, info); ``` could be modified not to perform on the workspace tags but only the tags from the current file or the current header (that is, from `info->file->tags_array` or `info->header->tags_array` (of course after performing tm_tags_find() on them before). That should speed up the search for local tags, current file's tags and header (and possibly include) tags significantly.
With regard to performance, I suggest that unless someone can provide a real world usage that demonstrates poor performance that we leave it as is and see how it goes.
Reasoning about performance with modern chips with multi-level caches, and optimising compilers is questionable to say the least, all the old rules of thumb are wrong, they were the first thing optimised away :smile:
There are regularly comments about string comparisons being "slow" (in lots of issues/PRs and not just Geany, not picking on @techee comment above) but modern compilers require `-fno-builtin` to _stop_ them emitting hardware optimised instructions that the chip maker developed to handle C's null terminated strings, utilising the caches, and interleaved memory channels, and prefetches etc. That old rule of thumb is pretty untrue these days and especially when the strings are likely to be less than a cache line long, like symbols are, they likely take one memory access and the actual comparison is hidden.
So unless an implementation has gross performance problems (like time is exponential in size) then unless we have an actual usage thats a problem, or benchmarks, then use the simplest to write and maintain implementation first.
@techee pushed 8 commits.
247703127225cd33d49888e7e77dcecbfb8ed2f8 Revert "Make validity of local variables configurable based on used language" 284ef935c14fca8846f1d110828db5f457051591 Rename is_valid_autocomplete_tag() to tm_workspace_is_autocomplete_tag() 879f6dd8215d9c89e9f6c589dddedf4fca6d5ba5 Require non-anonymous tags in tm_workspace_is_autocomplete_tag() 0567b156d380093c485b8138cf9260d6907c4ca8 Simplify predicate functions 40e09824385abc633f69284d350be6ecd190bd48 Search in file tag arrays for non-scope autocompletion when applicable 6d7e9e80e547c55d7d8658d99c9a4428899acce7 Remove unnecessary "lang" parameter of various functions 116c9a608db185b8237b4be068bfe11cd4dab536 Move tm_parser_langs_compatible() to tm_workspace_is_autocomplete_tag() e7afb76fb5d9c611f669f0f4dfe041bbc9ef445b Remove code related to searching tags in header files
I don't want to cause some tensions here with my PRs and don't want to introduce code others don't agree with so I removed the header-related code. I believe I also addressed all the other comments @kugel- had, improved the performance of non-scope autocompletion by searching in per-file tag arrays when applicable and made a few cleanups.
@kugel- Does it look good to you? What about the proposal related to handling header files (and possibly include files) by having a hash map in `TMWorkspace` for fast lookups based on a file name?
@elextr I agree with most of the stuff about performance and I think we shouldn't do any crazy optimizations. On the other hand, I don't feel comfortable when we do some things unnecessarily and there's a simple fix like in this case.
@techee now you have upset me, and I am the one helping you, not the person who comes in at the last minute and demands changes for what appear to be purely procedural reasons. I have instead explained why I do not support reducing the capability but been ignored. If you are going to make wholesale changes please consult others involved, yes my time zone often means some delay (1/2 - 1 day), but its better than just making wholesale changes without consulting. [end gripe]
Anyway it appears that there is more improvement to be had, even before the latest changes reduced it, which I havn't tested. Some of this didn't happen to be visible in previous testing because I was (un)lucky and alphabetic sorting appeared to be "doing the right thing".
# Background
Most of the functionality of a C++ application is packaged in classes, so most declarations are members, even many objects that would be a `static` in C are static members in C++.
And since most data is now members, autocompletion of member names is now more important, its likely as common as autocompletion of local variables.
But in the code of member functions, member names are not qualified, so they are non-scope autocompletion. This is unlike C where struct members are always scope autocompletion of a variable `.` or a pointer `->`, or Python where members are qualified by `self` in member functions.
So the quality of non-scope autocompletion is a significant determinant of the user experience.
Also for C++, more is declared in the header files and referenced in the body files than with C, particularly all those members as outlined above.
Geany and ctags do not do include file tracing because they do not know the build options that usually define where those files are found. So the heuristic that tags from files of the same name but different extension are headers is a reasonable compromise.
# Autocompletion
IIUC this PR uses heuristics to identify the likely type of the base for scope autocompletion, including prioritising local variables by position in the current file, members of the class inside a member function of the class and not outside one, then (before the last changes) symbols in header files determined using the above heuristic, then globals from all open files and loaded tags (IIUC).
As said [here](https://github.com/geany/geany/pull/3175#issuecomment-1107831415) non-scope autocompletion sorts local variables to the start of the list, but lumps the rest alphabetically, including all those members that are so important. But this is the case that presents the user with a list which now includes members that are invalid, and valid members mixed into globals.
So in one case a reasonable heuristic based on prioritisation is used, and in the other case its not, and that is the case that is visible to the user. Since the autocomplete list generation already includes multi-level sorting for locals, why not use the same prioritisation as used for the scope autocomplete to order the list. Or at least separate members like locals.
If important functionality is removed from this PR either Geany will still give a bad user experience for C++ autocomplete, or another PR must be immediately applied to scrape off some of the warts, so whats the point in separating it?
I appreciate the split, thanks
@elextr IMO a PR doesn't have to be wholesale, especially not if it gets large and unwieldy. Development is done incrementally and this may reflect in multiple PRs until the feature is completed.
Same as with my "session split" effort that spans multiple PR (would be a neverending story if I tried to make a single, wholesale PR).
@elextr @kugel- I'm really open to any course of action, it's just really hard to satisfy both of you at the same time. My personal preference would also be to merge this PR as it was (because it was reasonably well tested and there was nothing terribly wrong with it I think and also because at this point with the commits above it will take time to rebase them, and in a subsequent PR add the header stuff again). In any case, you two should find some agreement and I'll prepare the PR in the corresponding way.
@kugel- I'd also like to discuss the stuff from here:
https://github.com/geany/geany/pull/3185#pullrequestreview-958518326
I just don't know whether it means you are against the patch completely or whether it would get approved by you under some conditions. Some things are unfixable by nature - it will be a C/C++ only thing and it will make some assumptions. Potential performance problems are fixable as I hinted in my previous posts. As @elextr said, it's a heuristic only to improve sorting so it doesn't have to be perfect and the results seem better with it than without it.
I'd also like to know your opinion on how it should be implemented so I don't spend time preparing a PR that doesn't get approved.
it's just really hard to satisfy both of you at the same time.
Be honest, its impossible :smile:
My personal preference would also be to merge this PR as it was (because it was reasonably well tested and there was nothing terribly wrong with it I think and also because at this point with the commits above it will take time to rebase them, and in a subsequent PR add the header stuff again).
Thats pretty much my argument. @kugel- yes it probably would have been better to do it in parts when it started, but when its done its just a waste of time and effort that you have not contributed to. I will not waste any effort on the split PR. @techee and I seem to agree on the preferable way forward, so I suggest that unless someone else chimes in then it be merged without the split.
@techee what I tried to say in the last post (sorry if its a bit incomprehensible, I was in a rush), its not as good as it could be, but I agree its an improvement on autocomplete without locals. Whichever way this goes ahead please make an issue with the other things that you think can be done, and I can add the above post to it as well.
@kugel- requested changes on this pull request.
Minor stuff.
The "walk class hierarchy up for scope completion" makes a tiny bit nervous but it's your call. It would have been better to also separate this out but I don't want you to kill me now so keep it :-)
foreach_ptr_array(tmtag, i, tags)
{ + if (!tm_workspace_is_autocomplete_tag(tmtag, doc->tm_file, current_line, current_scope))
Can you add a comment why this condition is needed now? It's not entirely clear to me in the context of "goto tag"
{
- TMTag **tag, *last = NULL; - guint i, count, num; + TMParserType lang = current_file ? current_file->lang : TM_PARSER_NONE; + + /* ignore local variables from other files/functions or after current line */ + gboolean valid = !(tag->type & tm_tag_local_var_t) || + (current_file == tag->file && + current_line >= tag->line &&
Out of curiosity, why did you drop the idea of making this language specific? I think @elextr had a good example why this might not work so well for python.
@techee commented on this pull request.
foreach_ptr_array(tmtag, i, tags)
{ + if (!tm_workspace_is_autocomplete_tag(tmtag, doc->tm_file, current_line, current_scope))
This is what you suggested doing here instead of having the condition duplicated: https://github.com/geany/geany/pull/3185#discussion_r862393644
@techee commented on this pull request.
{
- TMTag **tag, *last = NULL; - guint i, count, num; + TMParserType lang = current_file ? current_file->lang : TM_PARSER_NONE; + + /* ignore local variables from other files/functions or after current line */ + gboolean valid = !(tag->type & tm_tag_local_var_t) || + (current_file == tag->file && + current_line >= tag->line &&
Because I think that even though it's syntactically valid, such cases are rare enough to justify seeing many more (invalid) local variables defined after the current line. But this is something that will have to be tested once Python local variables are enabled.
@kugel- I'm still waiting for your reply regarding how you imagine the header patch should be implemented and until you sort out with @elextr how to proceed with this PR. I'm not going to do any more work regarding this PR until this is resolved because it's really a wasted effort for me.
@kugel- commented on this pull request.
foreach_ptr_array(tmtag, i, tags)
{ + if (!tm_workspace_is_autocomplete_tag(tmtag, doc->tm_file, current_line, current_scope))
Right, I suggested to refactor the added if clause and you did a good job there. But now I'm puzzled why this new condition is relevant at all at this place.
@kugel- commented on this pull request.
{
- TMTag **tag, *last = NULL; - guint i, count, num; + TMParserType lang = current_file ? current_file->lang : TM_PARSER_NONE; + + /* ignore local variables from other files/functions or after current line */ + gboolean valid = !(tag->type & tm_tag_local_var_t) || + (current_file == tag->file && + current_line >= tag->line &&
OK
I'm not fundamentally opposed to the header patch (and I never that something like that). But I have concerns about the exact requirements and implementation that are better dealt with in a separate PR, IMO. It became clear to me that it adds a lot of benefit and @elextr totally wants to have it so I want to make sure the implementation is solid when we merge it.
I truly don't understand why it causes so much friction when I ask to separate out an unrelated feature. I really only want to focus on "completing local variable" for this PR so we can get it in ASAP (I already failed at that goal because it probably would be merged already if I just kept shut up) and then improve user experience through other means separately. If you think that I would want to reject one PR or another then that's really not the case. But given my limited time to actually review code changes I really want to focus on individual PRs (ideally with reasonably small diffs) because I can't accept overloaded PRs with confidence.
Also I did mean it honest when I said that @elextr and you can deal with this PR without my involvement. I'm OK as long as there is one other person that can assess the changes. Ultimately, I'm even OK with merging PRs without review if it's really impossible to get any feedback (not uncommon these days, unfortunately) though it come with a kind of warning in advance.
@techee commented on this pull request.
foreach_ptr_array(tmtag, i, tags)
{ + if (!tm_workspace_is_autocomplete_tag(tmtag, doc->tm_file, current_line, current_scope))
This is because for tag definition/declaration goto basically the same conditions like for autocompletion apply. Without this patch, if you perform the goto for e.g. variable `i`, you'd see all the local variables `i`'s in your project, which is a lot and irrelevant to the current function.
Maybe `tm_workspace_is_autocomplete_tag()` could be named a different way, I just couldn't come up with a better name.
To be clear, no hard feelings here, I just want to progress with this PR so everybody is happy. I'm pretty flexible with how to proceed, some extra work is fine with me, but I don't want to do the ping pong that guy A says "do it this way", I do it, and guy B says "don't do it that way".
Similarly with the header patch - if I open a separate PR for it, I'd like to prepare it in such a way that it's acceptable for everyone so that I don't have to rework it again. My current proposal is to have a hash table in `TMWorkspace` mapping `short_name` (filename without path) to the corresponding `TMSourceFile`, enable include tag generation and sort the tags that those from the included files are before the rest of the workspace tags (with highest priority given to the header having the same name as the current file). This avoids the hard-coded file extensions and the lookup should be fast thanks to the hash table. Does this proposal sound as a good way to implement it?
Please, if I wanted to discuss the in this PR, I wouldn't have asked to separate it out. To be honest I'm not even sure if simply looking for the header is the right approach. I really don't know the requirements but this approach it seems to encode a lot of assumptions. For example, if the requirement is to autocomplete class members then I would search for tags within the class, regardless of which header file (if any!) contains that class declaration. I really want to be clear on the requirements along with the implementation.
If you don't want to open the PR yet, that is completely fine. Then I suggest we discuss the requirements in an issue or through some other means, just not in this PR.
Please, if I wanted to discuss the in this PR, I wouldn't have asked to separate it out.
This is the core of the issue for me - your "I don't want to discuss it" (and possible subsequent "prepare another PR and I'll nix it") is maybe comfortable for you but is much more work for me and since we will have to discuss it anyway, I don't want to spend my time implementing something which doesn't get accepted.
I also don't agree this isn't relevant to this PR - it affects the ordering of symbols which is important both for scope and non-scope autocompletion and the result of the discussion affects how autocompletion works which is the main part of this PR.
So once again, I'd like to have it discussed now. I could create a separate issue for it but since it's so tightly coupled to this PR, I don't see a reason why not to discuss it right here.
For example, if the requirement is to autocomplete class members then I would search for tags within the class, regardless of which header file (if any!) contains that class declaration.
That's not what this is used for - scope autocompletion does work the way you describe.
For **non-scope** autocompletion (aka "the simple one returning everything") this serves for improved ordering of symbols. If you meant you'd also search for classes like in the scope autocompletion, I don't really know how I'd do something like that efficiently. One could probably do some hybrid autocompletion where the non-scope autocompletion would also do scope autocompletion stuff as part of it but then you'd have to somehow have to merge it with the rest of the autocompletion symbols and this would have to be reasonably efficient. Right now I don't feel like I'd want to rewrite non-scope autocompletion in some major way like this.
But it's not only about members of classes but also other symbols defined in the headers - and I just don't see what's wrong if symbols from the included headers will be higher in the list than the rest of the symbols. Or in other words, now the symbols are sorted just alphabetically and alphabetic ordering has no relation to how relevant the given symbol is to the current file. Wouldn't ordering the symbols based on whether the symbol comes from the included file improve the situation?
For **scope autocompletion**. Imagine you have 2 identically named symbols `name` of different (structured) types defined in two different files - totally common for big projects. If you pre-order the symbols so that those from included files are examined first, you increase the likelihood that the one you are going to examine first is the right one. Is the result guaranteed to be perfect? Not. Is it better than picking a random symbol from workspace called `name`? Definitely.
My reviewing capacity is very limited and I don't want to discuss with you endlessly about procedural concerns. This is getting rather tragical and burns much more time and mental energy than any additional issue/pr that might come about.
It's your choice, we can just take the current state and merge it (after some squashing) and deal headers next or you get this in with @elextr who was pretty happy with the earlier state. I'm not going to engage in further discussions if the header patch should be part of this PR or not, my time is better spend reviewing actual patches.
I don't want to spend my time implementing something which doesn't get accepted.
I'm sorry that you're still fearing this. I tried my best to not give the impression that I would outright reject the header patch. As with any other patch, I'll accept it if it's beneficial and has a solid implementation. But I do want to asses (series of) patches in as small portions as possible (given my limited review capacity, e.g. this allows me to review in the train when I go to work) and not lumped together in huge wholesale PRs, even if that means that Geany's master branch is temporarily incomplete.
I am not really interested in this PR in its current regressed state.
It's your choice, we can just take the current state and merge it (after some squashing) and deal headers next or you get this in with @elextr who was pretty happy with the earlier state. I'm not going to engage in further discussions if the header patch should be part of this PR or not, my time is better spend reviewing actual patches.
@kugel- so your approach is to smash and run. You demand major changes just before merge so the PR is no longer in the state where I was pretty happy with it, completely wasting my previous work of test and inspect. And now you want to cry time poor when asked to provide discussion of the change you demanded. Everybody is time poor, none of us are full time Geanyists, so forcing extra work on people without contributing simply makes you rude and damaging to the project. I am also still waiting on replies to my comments on your own PRs #3178 and #1813, clearly other peoples contributions don't matter to you.
@techee as I said before I am disappointed that you chose to regress this PR before discussion with someone who is helping you, so wasting their efforts. @kugel- is right in part, significant changes should be discussed on issues before PRs are made, then multiple partial PRs make sense and they can be smaller without large numbers of commits that need complex squashing at merge time, they should be able to be squashed flat at merge (which can't introduce bugs). But big changes mid PR is a waste of everybodys efforts, do not do that without discussing it with them.
To you both, as I have said before, I do not use Geany for C++, the state of this PR does not help me, I was just trying to help @techee provide as useful a user experience for those who do so. So completely disregarding that effort by demanding and making wholesale changes to the PR without any discussion has left me unhappy about engaging any further with this PR.
@kugel- correctly pointed out that there is a lack of effort available to the project ATM and incidents like this don't help, as he correctly said they suck up effort. More discussion and planning is needed, and it does not help that contributors are in different time zones, and I appreciate the extra efforts of those for whom English is not their first language, but it is what it is.
Ultimately, I'm even OK with merging PRs without review if it's really impossible to get any feedback (not uncommon these days, unfortunately) though it come with a kind of warning in advance.
As I noted above you have not replied to feedback on several PRs, however I agree with the sentiment, the process for simple bug fixes should be simple, an OP should be allowed to merge their own simple PRs providing warning and sufficient time is allowed (I suggest two weeks, including two weekends) if no feedback is received. But additional features, or changes to existing features really need to be discussed more, not less, to avoid more of this sort of occurrence. One thing I definitely agree with @kugel- is that discussions over code is not the best way to do it.
So how to progress this PR now.
As @kugel- has said he is not blocking it whichever way, and I am no longer interested in pushing it, so that leaves @techee to decide. Which is fair enough @techee has done all the work.
I guess @kugel- can't build and test on the train so if @techee gets the PR into an immediately mergable state, to get progress I will do a quick scan, and build and test and if ok press the green button. But @techee whichever state you decide on, please document the intended behaviour so I know what to test.
But @techee whichever state you decide on, please document the intended behaviour so I know what to test.
Many observers will have been astonished that I haven't mentioned my favourite topic (documentation) so far. Thats because any attempt to describe the behaviour has seemed wordy, complex, and confusing. But I have had an idea which @techee can use to do the above, and the manual at the same time :smile:
It appears that using tables[^1] in an appendix will make it clearer (I am not sure the contents are right, @techee to fix when he decides the version to be merged):
# Autocompletion behaviour
## C++ Autocompletion[^2]
| category of symbol | autocomplete list | base for scope completion | | --- | --- | --- | | function local declared before the cursor | sorted to first group then alphabetically | prioritised first, closest declaration wins | | function local declared after the cursor | sorted to first group then alphabetically | not considered | | data member of class parent to function | sorted to second group then alphabetically | prioritised second | | declaration in header of current file | sorted last group then alphabetically | prioritised third | | other symbol | sorted last group then alphabetically | prioritised last |
In the Geany manual I would put the tables for each language in an appendix and refer to it from the [autocompletion](https://www.geany.org/manual/current/index.html#autocompletion) section with a sentence like "For some languages the autocompletion list is ordered by heuristics to attempt to show names that are more likely to be what the user wants close to the top of the list. See [Autocompletion behaviour] for the list ordering used for languages where it is applied."
And in the scope autocompletion section replace the last sentence with "Most languages only parse global definitions and so scope autocompletion will not work for names declared in local scope. A few languages parse both local and global symbols and this makes it more likely that the base name (eg `this_name->`) occurs more than once in the available symbols meaning possible scope completions from several types could be mixed. Geany does not have exact symbol scoping and visibility information to resolve which definition is to be used, so to disambiguate this situation Geany uses a heuristic described in [Autocompletion behaviour] to prioritise the possible completions which are most likely to be what the user wants."
[^1]: sorry its markdown not rest, but I wanted it to show in github [^2]: C++ is not C, so they are different, it is left as an exercise for the reader to make the C table, and in future the Python table
@kugel- commented on this pull request.
foreach_ptr_array(tmtag, i, tags)
{ + if (!tm_workspace_is_autocomplete_tag(tmtag, doc->tm_file, current_line, current_scope))
So "goto tag" is now constrained to the same tags as scope completion, because with all the local tags of other functions that would be too much? Can you just add this as a small comment please so this call doesn't look out-of-place?
I am also still waiting on replies to my comments on your own PRs https://github.com/geany/geany/pull/3178 and https://github.com/geany/geany/pull/1813, clearly other peoples contributions don't matter to you.
This PR is currently taking all my Geany-time, it's simple as that. Besides, I already made the suggested code changes to #3178, I only failed to notify you with an explicit comment (fixed that now).
@kugel- so your approach is to smash and run.
Please don't make rude accusation like that. I do not run, I'm here. I'm just trying to get this PR into a shape that I can review with confidence. It's in that state now, and I'm accepting the changes (aside from the minor comment request). I'm not going to respond to the other false accusation in your post, lets focus on getting this over the finish line.
Many observers will have been astonished that I haven't mentioned my favourite topic (documentation) so far
Good call! I like your suggestion. Normally I would say documentation ought to be part of the PR that implements it, but given that a) this PR is already heated such that I would rather get over it (by merging) and b) there is more to come that affects the documentation, I may suggest to deal with that at a later point.
@kugel- great, if you are going to help @techee I'll stay out of it.
I am not really interested in this PR in its current regressed state.
Pretty much my thoughts too and I think I'm leaving this PR at least for now. It's not really clear to me which way to continue and the kind of discussion here just sucks my energy.
In any case, if anyone wants to take over this PR, I'm here to help. And even if someone wants to implement autocompletion in a different way (there are of course tons of ways to implement it), this can serve as a starting point or some inspiration.
Oh my god, this is probably the worst possible outcome. I'm deeply sorry that I caused so much friction that this PR is dead now. I think the feature doesn't deserve this end.
I'm OK with merging the current state, after some squashing, and then improve upon it based on Geany master. Is that not OK for you? Alternatively, restore the state @elextr enjoys and get his approval?
I'd be hesitant to merge it myself since you guys say this PR alone gives a poor user experience and nobody commits to do further work after leaving the sinking ship.
I'm OK with merging the current state, after some squashing, and then improve upon it based on Geany master. Is that not OK for you? Alternatively, restore the state @elextr enjoys and get his approval?
No problem here, I'm totally fine with whatever the outcome is, probably best to wait for @elextr 's opinion tough.
I have said above, when I thought @kugel- was not willing to work on this any more, I am willing to merge any state after testing, but I wanted to know what is supposed to work in the version being tested since an unknown amount of functionality was removed.
But since @kugel- did not seem to think it was needed to know what to test, and is still working on this, I leave it to you two to execute the merge and improve process and call me when you have a documented version available for test. If an issue is opened to discuss the improvements I will comment from a C++ point of view since it is not C and is not used the same.
So it seems we settled on merging the current state (again, some squashing is IMO required) and get more testing on the master branch as well as making further improvements along the way?
@techee can we finish this, what else do you need?
@techee pushed 2 commits.
745cb085449f7946299bb52de3b1aafd3612868f Add comment about tm_workspace_is_autocomplete_tag() usage in tag goto 9aca7051dcf28932e8997040fadd0346a48b3393 Update documentation related to scope autocompletion
@techee commented on this pull request.
foreach_ptr_array(tmtag, i, tags)
{ + if (!tm_workspace_is_autocomplete_tag(tmtag, doc->tm_file, current_line, current_scope))
Done.
So it seems we settled on merging the current state (again, some squashing is IMO required) and get more testing on the master branch as well as making further improvements along the way?
I'm not quite sure we settled on something which is why I mostly abandoned this PR ;-).
@techee can we finish this, what else do you need?
Basically from the code-related things, unless I don't overlook something, the remaining stuff was the missing comment I just added and also documentation. Regarding documentation, I changed what @elextr proposed to something a bit different - will add an explanation in the next post.
It appears that using tables[1](https://github.com/geany/geany/pull/3185#user-content-fn-1-c28162a939768f395...) in an appendix will make it clearer (I am not sure the contents are right, @techee to fix when he decides the version to be merged):
Despite everyone's love to tables, graphs and similar cool stuff, I believe the table is an overkill here. The current sorting is not something that has to be preserved, could change in the future if we find a better heuristic, and it's just a proxy to "show the most relevant symbols first". So
For some languages the autocompletion list is ordered by heuristics to attempt to show names that are more likely to be what the user wants close to the top of the list.
is completely sufficient IMO.
The scope autocompletion part:
Most languages only parse global definitions and so scope autocompletion will not work for names declared in local scope. A few languages parse both local and global symbols and this makes it more likely that the base name (eg this_name->) occurs more than once in the available symbols meaning possible scope completions from several types could be mixed. Geany does not have exact symbol scoping and visibility information to resolve which definition is to be used, so to disambiguate this situation Geany uses a heuristic described in [Autocompletion behaviour] to prioritise the possible completions which are most likely to be what the user wants.
isn't correct. In scope autocompletion we always get all the members sorted alphabetically - when you think of e.g. members of a struct or class, these are defined in a single file and we have no guess which of the members the user wants.
So I used this wording instead:
Most languages only parse global definitions and so scope autocompletion will not work for names declared in local scope (e.g. inside functions). A few languages parse both local and global symbols (e.g. C/C++ parsers) and for these parsers scope autocompletion works also for local variables.
So if you squash a bit (especially reverts of earlier commits) then I'll immediately approve.
The documentation is good enough for me. If there's any desire to improve it it can be done later on, IMO. I don't feel such desire right now.
OK (if others are OK with it).
I'm not sure how much time I'll have in the coming weeks so it may be a bit delayed on my side.
@techee
OK (if others are OK with it).
As I won't get any benefit from this PR since my C++ is done elsewhere I won't waste any more time on it.
Suffice to say that until the last of the changes (those that have now been removed) there was no real improvement in C++ usability, and in fact it was possibly worse due to more noise in autocompletes. @techee I am sorry to have wasted your time on trying to improve it.
@kugel- I do not know if it adds any worthwhile capabilities to C, I only tested lots of C++. Its up to you to merge it if you think it adds anything useful, I won't.
Suffice to say that until the last of the changes (those that have now been removed) there was no real improvement in C++ usability, and in fact it was possibly worse due to more noise in autocompletes. @techee I am sorry to have wasted your time on trying to improve it.
Hmm, but if nothing else, wasn't it at least useful to see local variables in the non-scope autocompletion popup (and placed at the top)? From my testing, it was the non-scope autocompletion which benefited most from this PR and started offering more relevant results thanks to the ordering.
The usefulness of the scope autocompletion may indeed be iffy since we see only the statically declared things and don't see things like `auto` variables or exact types of virtual variables. Still, it seemed to work quite well with Scintilla sources (which doesn't use `auto`'s and doesn't use too many dynamic C++ features) from my limited testing.
Hmm, but if nothing else, wasn't it at least useful to see local variables in the non-scope autocompletion popup.
Its now stretching my ... erm ... erm .. oh yeah memory!!! But IIRC:
Maybe its the way different C++ is written, but in the range of C++ I tested most code was in class member functions, and class data member names were the most common, but they were declared in the class definitions, which were in the header files and so without header preferential sorting they were buried down in the alphabetic part of the autocomplete, which grew significantly. Hence my dissatisfaction at its removal. Locals were second most common, but mostly short so they didn't even trigger 4 character autocomplete, so it didn't help that much with those[^1]. Also many locals were declared within `for()` and `if()`, I don't remember if ctags saw them.[^2]
The main codes I tested had a certain amount of repetition between classes, and since its not indicated where names in the Geany autocomplete list come from, my inference about why things appeared where they did may be incorrect. Since that testing I have used vscode for some short experimental coding where I didn't bother to set up build info, and the autocompletes show a similar list to what I remember getting with the header file sorting enabled[^3]
As I suggested C (or Cish C++) may get greater benefit from locals, since it doesn't have class member names that are visible in member functions without any qualification. And older C codes with the "declare at the top" idiom may tend to longer local names, although newer C following "don't declare until its initialise-able" may tend to shorter names and smaller scopes.
[^1]: an unscientific survey says the most common locals are `i` for array/vector indexes and `p` for a pointer, not really expecting autocomplete to help with those ;-) [^2]: a common C++ idiom where p is a pointer is `if(auto p = expression; p){ use p->whatever safely; }` and `p`s scope is just the `if` body, or `for(auto& x : collection){ use x.whatever only in this scope }` [^3]: yep vscode without build info has the same problems as Geany and seems to give similar crap, thats comforting for Geany in one way, but vscode sorting heuristics seem to make locals and members come near the top, although its not perfect either. But it shows where the currently selected name comes from so I can check, and vscode seems to correctly put the most relevant of the repeated sources of the same name near the top of the list. I get the impression that it has more understanding of scope. Also it does appear to read which stdlib include files (the `<ones>`) are included and only shows global stuff from those, or stuff made visible by `using namespace std;` statements. Eclipse with full meson build info seems to be much more accurate.
yep vscode without build info has the same problems as Geany
Dammit, I have to apologise to vscode, my experimental code used several `<cthing>` headers like `<cstddef>` which are C++ified versions of the equivalent C header. But they still dump stuff in global namespace like C does (as well as politely in `std::`). Why C++ technical committee?? What does that achieve? Ahem, anyway vscode is just doing what it was told.
@techee Hey. Any progress on this? Just some squashing was requested from my side.
@kugel- Yeah, finally done, see #3266.
@kugel- pushed 11 commits.
8ff56a5c779ffcccef0ceb0b412be62f606c76e1 Rename tm_tag_file_t to tm_tag_local_var_t d5cc1d05fb82218d01f1d61a9329c09a43cf0d77 Enable local tag generation for C/C++ 9e4ef229a666b0aae746b964514d8ecdb12c9d37 Update (non-scope) autocompletion to take into account local variables 25f150931e376d57654fb1df59b892c48af80e5a Update scope completion to take into account local variables 13bdb37cf7496a27b0b77c9893839c0fc3aba186 Update goto symbol definitions to take into account local variables 2cf2e87bda17de13fea495b965816b9c19aec920 Strip more things from variable type e4f2ed745238708673bef4c9a65e5e933f2215b2 Support (multiple) inheritance by scope completion 2a5da2225f7aa87bdc7f82c3966f6fb32754ae5c Rewrite member_at_method_scope() to handle more situations 4f45f7014b3e7aabf650a68153ea38db4ffc5fcb Sort non-scope autocompletion results by locality db1c862d5efc3a6a8aadf4232dda2d0093455f00 Remove unnecessary "lang" parameter of various functions a91a9c6ca17e5a7a163b095d5a9065afa7e8af77 Update documentation related to scope autocompletion
As disussed on https://github.com/geany/geany/pull/3266 I pushed a slightly-modified history. The diff is the same all around. Unless anyone speaks up that can be merged in my opinion.
@kugel- approved this pull request.
Merged #3185 into master.
Great, thanks!
github-comments@lists.geany.org