These are some patches related to the scope completion patch set:
1. The first patch is the scope completion for namespaces. 2. Then there's a patch using the langs_compatible() function for scintilla type colorization - this is a prerequisite of https://github.com/geany/geany/pull/857 3. When working on the above patch I noticed that the -1/-2 language type codes are a bit confusing and not used correctly in the function so I tried to clean it up a bit and fix things. 4. When testing scope completion for boost I noticed there are many tags with identical names (probably because of inheritance). Just one tag of the given name can be passed to Scintilla for tag colorization to improve performance a bit. 5. The very last patch is a suggestion but I can remove it if you find it too intrusive. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/906
-- Commit Summary --
* Add scope completion for namespaces * Cleanup NONE/AUTO filetype definitions * Use the langs_compatible() function when passing typenames to scintilla * Don't pass multiple copies of identical type name to scintilla for colorization * Don't use ctags types inside Geany
-- File Changes --
M src/editor.c (12) M src/filetypes.c (2) M src/filetypes.h (8) M src/symbols.c (10) M tagmanager/src/tm_parser.h (14) M tagmanager/src/tm_source_file.c (23) M tagmanager/src/tm_source_file.h (12) M tagmanager/src/tm_tag.c (22) M tagmanager/src/tm_tag.h (8) M tagmanager/src/tm_workspace.c (108) M tagmanager/src/tm_workspace.h (8)
-- Patch Links --
https://github.com/geany/geany/pull/906.patch https://github.com/geany/geany/pull/906.diff
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906
Not yet tested, but LGTM -- apart maybe the API break, but well.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-185755616
When testing scope completion for boost I noticed there are many tags with identical names (probably because of inheritance). Just one tag of the given name can be passed to Scintilla for tag colorization to improve performance a bit.
C++ allows functions to have the same name but distinguished by different parameter types, and this is used a lot in Boost (and STL), so good idea.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-185944956
@elextr @codebrainz what do you think about the (technical) API break? I guess I'd add something like 18f013832817c2be347394ec0a40b85abca6ddd7 (from [b4n@wip/techee/scope_fallout](/b4n/geany/commits/wip/techee/scope_fallout)) to be on the safe side, but maybe it's just useless and more work…
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-186632316
@@ -14,14 +14,12 @@ #include <glib.h> #include <glib-object.h>
+#include "tm_parser.h"
hum, this actually breaks every plugins because *tm_parser.h* isn't installed, so it gives nice errors like `fatal error: tm_parser.h: No such file or directory`
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906/files#r53552439
@@ -14,14 +14,12 @@ #include <glib.h> #include <glib-object.h>
+#include "tm_parser.h"
I tested it over existing installation so I missed this. The tm_parser.h file can easily be made part of the installation so if others find the API "break" acceptable I'll update the patch to install the file.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906/files#r53553882
@@ -14,14 +14,12 @@ #include <glib.h> #include <glib-object.h>
+#include "tm_parser.h"
it's probably not as nice, but as the type is a mere typedef anyway it could "just" go in some other installed file instead of *tm_parser.h* if we don't want to expose more stuff
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906/files#r53553945
@@ -14,14 +14,12 @@ #include <glib.h> #include <glib-object.h>
+#include "tm_parser.h"
Yeah, that's an option too - on the other hand lang in tm_source_file.h and filetypes.h is public, including docstring, and then its value is just some magic constant.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906/files#r53554074
@@ -14,14 +14,12 @@ #include <glib.h> #include <glib-object.h>
+#include "tm_parser.h"
fair point
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906/files#r53554368
Well, if nothing uses that API, that is none of:
1. Geany's core plugins 2. Geany-plugins 3. any of the external plugins listed on the g-p website
Then why is it there?
On 21 February 2016 at 01:48, Colomban Wendling notifications@github.com wrote:
@elextr https://github.com/elextr @codebrainz https://github.com/codebrainz what do you think about the (technical) API break? I guess I'd add something like 18f0138 https://github.com/geany/geany/commit/18f013832817c2be347394ec0a40b85abca6ddd7 (from b4n@wip/techee/scope_fallout http:///b4n/geany/commits/wip/techee/scope_fallout) to be on the safe side, but maybe it's just useless and more work…
— Reply to this email directly or view it on GitHub https://github.com/geany/geany/pull/906#issuecomment-186632316.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-186705608
@elextr The value is part of various structures used by Geany:
https://github.com/techee/geany/commit/b8187319add15e829dbce84b1190e34bcce21...
https://github.com/techee/geany/commit/b8187319add15e829dbce84b1190e34bcce21...
https://github.com/techee/geany/commit/b8187319add15e829dbce84b1190e34bcce21...
It would be possible to split them into public/private parts but this would break ABI. And I think the language used for parsing might be potentially interesting for plugins too so breaking the ABI doesn't seem to be worth it.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-186853077
An API break is questionable, but a "nice to have" change like this PR is not worth an ABI break.
Make the comment a non-doxygen one that says "This field is not meant to be used in plugins, you have been warned" :)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-186998755
@b4n What do you suggest to do? I can remove the doxygen comment as Lex suggests - but at that point lang won't be part of Geany API any more so I could do the renaming without breaking API (there's no reasonable way to use the API as it is now because the language enum isn't accessible so I don't think it's risky in any way). Or keep the doxygen comment, perform the renaming and export the language enum. Or keep langType as it is. Which one do you prefer?
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-188480182
An API break is questionable, but a "nice to have" change like this PR is not worth an ABI break.
How would this break ABI? It's a typedef rename, not a type change. The "nice to have" part is including `TM_PARSER_*` enum values.
[…] Which one do you prefer?
Hum… I prefer the new type name. I prefer full compatibility. I prefer stuff to be defined where it makes sense.
So, I guess what I'd prefer is * use the new type name * introduce a deprecated alias of the old name * install *tm_parser.h* (possibly guarding everything but the typedef in `GEANY_PRIVATE`)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-188513167
(and possibly removing the doc comment as @elextr suggested if it was never useful and isn't used, so we don't advertize it anymore -- yet technically keep compatibility)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-188513482
BTW, one potentially legitimate use of the `lang` member (and the `langType` type, possibly) would be comparing two tag's language and see if they are the same or not. One could imagine something like that to recognize whether tags were generated by i.e. the C parser even for a custom filetype (`if (filetypes[GEANY_FILETYPES_C]->lang == possibly_custom_ft->lang)`) or something similar. Not saying it actually makes sense, but it doesn't seem absolutely crazy.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-188514507
On 25 February 2016 at 09:26, Colomban Wendling notifications@github.com wrote:
An API break is questionable, but a "nice to have" change like this PR is not worth an ABI break.
How would this break ABI? It's a typedef rename, not a type change. The "nice to have" part is including TM_PARSER_* enum values.
One option was to move it into a GEANY_PRIVATE struct, that would change the struct and so break the ABI.
[…] Which one do you prefer?
Hum… I prefer the new type name. I prefer full compatibility. I prefer stuff to be defined where it makes sense.
So, I guess what I'd prefer is
- use the new type name
- introduce a deprecated alias of the old name
- install *tm_parser.h* (possibly guarding everything but the typedef
in GEANY_PRIVATE)
— Reply to this email directly or view it on GitHub https://github.com/geany/geany/pull/906#issuecomment-188513167.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-188552606
One could imagine something like that to recognize whether tags were generated by i.e. the C parser even for a custom filetype (if (filetypes[GEANY_FILETYPES_C]->lang == possibly_custom_ft->lang)) or something similar.
Yeah. But actually this case won't be broken by the API change - one would have to first assign the value to a variable of the langType type.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-189055116
So, I guess what I'd prefer is
use the new type name
Good.
introduce a deprecated alias of the old name
The trouble is that langType would be defined in both ctags and in the header and we'd have to keep using the LIBCTAGS_DEFINED macro tricks I wanted to eventually eliminate (at the moment still needed for tagEntryInfo). And because using the lang field in the various structs isn't that meaningful and because none of the gp plugins use it (and most plugins don't use the TM stuff at all which I think is the case of external plugins too), I think it's quite safe to assume nobody uses it.
But of course can add it if you disagree.
install tm_parser.h (possibly guarding everything but the typedef in GEANY_PRIVATE)
Done.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-189059814
The trouble is that langType would be defined in both ctags and in the header and we'd have to keep using the LIBCTAGS_DEFINED macro tricks I wanted to eventually eliminate (at the moment still needed for tagEntryInfo). And because using the lang field in the various structs isn't that meaningful and because none of the gp plugins use it (and most plugins don't use the TM stuff at all which I think is the case of external plugins too), I think it's quite safe to assume nobody uses it.
Myeah, as much as I don't like breaking API, I guess it's actually fine. And we never actually advertized the type, although as we did so for a member of this type it was kinda implicit, but well. Yet apparently nobody uses this. So it @elextr and @codebrainz are happy with it, let's break everything and pretend we did not :)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-189765981
Yeah, API change is ok, but why "pretend we did not"? Just increment the API, and if some smarty uses it, they can check the API and define the typedef themselves so to support both versions.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-189766415
@elextr Yeah, why not.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-189891510
@codebrainz @elextr @techee Do we agree on the API thing? If we do I'd like to merge this, which I'm using for some time and is great AFAICS -- and not that intrusive actually.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-191820559
@b4n I've written the thing so I agree :-)
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-191830458
LGTM
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-192026735
That confusing langType has had me searching around the sources looking for it on a number of occasions, so +1 for using a decent name. It'd be nice to eventually do the same for filetype_id type which has also confused on a few bindings projects.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-192042351
@codebrainz Yeah, filetype_id looks pretty alien too. I replaced its occurrences and deprecated the macro here:
https://github.com/geany/geany/pull/931 https://github.com/geany/geany-plugins/pull/381
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#issuecomment-192233465
Merged #906.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/906#event-578227791
github-comments@lists.geany.org