[geany] TM cleanups (#957)

List overview All Threads

newer

older

[geany] Improve Goto Symbol popup...

[geany/geany] TM: Fix actually...

Jiří Techet

10 Mar 2016 10 Mar '16

5:41 p.m.

This is hopefully the last "big" patch set related to TM. In fact, despite the diff size, it's actually not that big - the diffs are mostly caused by moving stuff around.

There are no functional changes in this patch set - just cleanups.

Most of the patches concentrate on TM<-->ctags interaction and simplification of their interfaces.

The last patch is an API/ABI break affecting projectorganizer and geanyprj - probably not worth the break alone but if there are more patches requiring ABI break, this one could be added. Can remove it from this patch set and submit separately.

It would be nice to have it applied early-ish in the 1.28 cycle as I'd like to do some work on syncing with upstream universal-ctags which will depend on these patches. You can view, comment on, or merge this pull request online at:

https://github.com/geany/geany/pull/957

-- Commit Summary --

* Remove the TagEntrySetArglistFunction hook * Initialize ctags at a single place instead of four * Improve ctags callback API * Use G_BEGIN_DECLS/G_END_DECLS * Remove unused tm_tagmanager.h * Move code related to various tag file formats into tm_source_file.c * Add an API-like ctags layer * Use TMParserType instead of language name in tm_source_file_new() * Clean up messy tm_workspace_create_global_tags() * Remove some unused return values and unnecessary checks

-- File Changes --

M src/document.c (5) M tagmanager/ctags/entry.c (8) M tagmanager/ctags/entry.h (1) M tagmanager/ctags/parse.c (9) M tagmanager/ctags/parse.h (7) M tagmanager/ctags/python.c (30) M tagmanager/src/Makefile.am (6) A tagmanager/src/tm_ctags_wrappers.c (113) A tagmanager/src/tm_ctags_wrappers.h (47) M tagmanager/src/tm_source_file.c (806) M tagmanager/src/tm_source_file.h (24) M tagmanager/src/tm_tag.c (559) M tagmanager/src/tm_tag.h (38) D tagmanager/src/tm_tagmanager.h (39) M tagmanager/src/tm_workspace.c (373) M tagmanager/src/tm_workspace.h (9) M wscript (1)

-- Patch Links --

https://github.com/geany/geany/pull/957.patch https://github.com/geany/geany/pull/957.diff

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957

Attachments:

attachment.htm (text/html — 4.9 KB)

Show replies by date

Colomban Wendling

10 Mar 10 Mar

6:24 p.m.

I looked quickly through it, and it looks quite nice, thanks :) I'll do a proper review later on, but yeah we should merge this early in 1.28.

@b4n is excited seeing this sets the foundations of greatly improving the interaction with CTags, and making things a lot clearer for everyone :)

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

Jiří Techet

6:35 p.m.

Great to hear :)

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

Jiří Techet

11 Mar 11 Mar

11:18 a.m.

I removed the API/ABI break patch - it's annoying because it causes ProjectOrganizer to crash without proper ABI bump. Will post it later.

Made some minor updates on the way.

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

Jiří Techet

22 Mar 22 Mar

4:19 p.m.

One more patch - this one finally moves the tag type assignment outside ctags parsers. Assuming it will be really annoying to map every single parser's tag types and sync ctags type definitions with universal-ctags definitions, I reserved the work on it to the time spent in train when going to/from Chemnitz Linux Days, free from any distractions like working Internet and similar. (The annoyingness really didn't disappoint.) I hope I didn't make any mistakes. I added some consistency checks to the code so typo-like and copy/paste-like errors should be caught. Anyway, happy reviewing :-P.

I'm pretty happy how things look after these patches - both TM and ctags are reasonably well separated from each other, ctags stuff isn't visible in Geany and TM looks the way I'd imagine it should have looked 10 years ago when it got used in Anjuta...

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-199887709

Jiří Techet

2 May 2 May

3:26 p.m.

New subject: [geany/geany] TM cleanups (#957)

@b4n Would you have some time to review this pull request? Would be nice to get it to the next release so I can start working on more ctags syncing.

There's nothing terribly complicated or tricky going on in the patches (unlike the previous scope completion stuff) - just the last one might be a bit annoying if you want to check all the mappings. On the other hand, the newly introduced mapping check should make sure there's no typo-like error and in addition, all the tests pass so there's probably no need to do any too detailed review of the patch.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-216266978

Colomban Wendling

3 May 3 May

5:20 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag)

Mmmwell, this is pretty specific to the Python parser and not well-contained, but well, probably still better than a whole new hook indeed.

BTW, this might be fixable in the parser side using the U-CTags' new Cork thingie? Not sure and not tested, but that would be nice.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/f61a64be29cbba0162f92eb5c1fe4a...

Colomban Wendling

5:27 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag) +{
guint i;

const char *parent_tag_name;

if (tag->type != tm_tag_method_t || tag->scope == NULL ||
g_strcmp0(tag->name, "__init__") != 0)
return;
parent_tag_name = strrchr(tag->scope, '.');

note: doesn't support the `/` separator previous code did, but our parser doesn't have it anyway. Also, it would only concern classes defined inside a function or method, which is less common.

Colomban Wendling

5:37 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

This is an odd type name for TM/Geany, we generally use CamelCase for those -- `TMCtagsCallback` or something?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/103d2c535862ae2682022dcfd714c2...

Colomban Wendling

5:49 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

It also seems to have a strange signature only targeted at our very specific usage, although it suggest it's meant to be a generic API. By that I mean the `invalidate` parameter, that seem more like a step indicator that might be better served as an enum, or at least rather name it something like "initialize" or something more generic.

But maybe ```C typedef enum TMCtagsParsingStep { TM_CTAGS_PARSING_STEP_INITIALIZE, TM_CTAGS_PARSING_STEP_EMIT_TAG, // ... possibly _FINALIZE too } ``` Or something. Or, if we can be sure `tag` is never `NULL` when emitting tags, rely only on this?

Also, documentation on this callback would be great :)

Colomban Wendling

6:10 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

return TAG_ACCESS_PRIVATE;
else if (0 == strcmp("friend", access))
return TAG_ACCESS_FRIEND;
else if (0 == strcmp("default", access))
return TAG_ACCESS_DEFAULT;
+#ifdef TM_DEBUG

g_warning("Unknown access type %s", access);

+#endif

return TAG_ACCESS_UNKNOWN;

+}

+/*

Initializes a TMTag structure with information from a tagEntryInfo struct

used by the ctags parsers. Note that the TMTag structure must be malloc()ed

before calling this function. This function is called by tm_tag_new() - you

not called by `tm_tag_new()` anymore

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/4dcc829e724a9018f92a8ef5201c2c...

Colomban Wendling

6:12 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

FILE *fp;

g_return_val_if_fail(tags_array && tags_file, FALSE);

fp = g_fopen(tags_file, "w");

if (!fp)
return FALSE;
fprintf(fp, "# format=tagmanager\n");

for (i = 0; i < tags_array->len; i++)

{
TMTag *tag = TM_TAG(tags_array->pdata[i]);
write_tag(tag, fp, tm_tag_attr_type_t
  | tm_tag_attr_scope_t | tm_tag_attr_arglist_t | tm_tag_attr_vartype_t
  | tm_tag_attr_pointer_t);

unrelated, but `write_tag()` return value should be handled

Colomban Wendling

6:15 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -45,6 +45,8 @@ typedef struct TMSourceFile

GType tm_source_file_get_type(void);

+void tm_source_file_ctags_init();

`(void)`

Colomban Wendling

6:16 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -72,6 +72,9 @@ static gboolean tm_create_workspace(void) theWorkspace->source_files = g_ptr_array_new(); theWorkspace->typename_array = g_ptr_array_new(); theWorkspace->global_typename_array = g_ptr_array_new();

tm_source_file_ctags_init();

shouldn't that be `tm_ctags_init()` instead of the `tm_source_file_ctags_init()` indirection?

Colomban Wendling

6:25 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

{

size_t dirty_len = strlen(includes[idx_inc]);

char *clean_path = g_malloc(dirty_len - 1);

```
for (i = 0; i < includes_count; i++)
```
```
{
```

	size_t dirty_len = strlen(includes[i]);

safety: `assert(dirty_len > 2)`-like

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/1c4aaa0eb72aa9e3c60cd604d4c20c...

Colomban Wendling

6:26 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

#endif

	if (!g_hash_table_lookup(includes_files_hash,

```
							globbuf.gl_pathv[idx_glob]))
```
```
	{
```

		char* file_name_copy = strdup(globbuf.gl_pathv[idx_glob]);

		g_hash_table_insert(includes_files_hash, file_name_copy,

```
							file_name_copy);
```

		if (!g_hash_table_lookup(table, globbuf.gl_pathv[idx_glob]))

```
		{
```

			gchar *file_name_copy = strdup(globbuf.gl_pathv[idx_glob]);

s/strdup/g_strdup/

Colomban Wendling

6:27 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

	char* file_name_copy = strdup(includes[idx_inc]);

	g_hash_table_insert(includes_files_hash, file_name_copy,

```
						file_name_copy);
```

	if (!g_hash_table_lookup(table, includes[i]))

```
	{
```

		gchar* file_name_copy = strdup(includes[i]);

s/`strdup`/`g_strdup`/

Colomban Wendling

6:30 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

if (!outf)
return NULL;
if (!tmp_errfile)

{
g_unlink(outf);
g_free(outf);
return NULL;
}

that's not correct, if `outf` failed to be created but not `tmp_errfile`, that last one is leaked. Unlikely I agreed, but still possible and probably worth fixing.

Colomban Wendling

6:36 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

}

-static void append_to_temp_file(FILE *fp, GList *file_list) +static gboolean append_to_temp_file(const gchar *outf, GList *file_list)

that function name is now a lie: it doesn't append anymore, but simply replaces the file's content.

Colomban Wendling

6:42 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -409,14 +413,21 @@ static void write_includes_file(FILE *fp, GList *includes_files) g_free(str); node = g_list_next(node); }

fclose(fp);

return TRUE;

`return fclose(fp) == 0;`

Colomban Wendling

6:43 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -437,6 +448,10 @@ static void append_to_temp_file(FILE *fp, GList *file_list) } node = g_list_next (node); }

fclose(fp);

return TRUE;

same, check write success

Colomban Wendling

6:46 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -128,7 +128,8 @@ extern void checkRegex (void);

/* Extra stuff for Tag Manager */ extern tagEntryFunction TagEntryFunction; -extern void setTagEntryFunction(tagEntryFunction entry_function); +extern void *TagEntryUserData;

exposing this is consistent with the function itself, but not used and "leaking" a few more ctags' guts. So if we could rather remove the `TagEntryFunction` global export than adding the data one, it'd be awesome :)

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/7be40f5832c082ca27bce5e4756c60...

Colomban Wendling

6:47 p.m.

New subject: [geany/geany] TM cleanups (#957)

I read every commit but the last one for the moment. Looks mostly great, see comments for the rest. I didn't test anything yet, though.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-216627020

Colomban Wendling

7:46 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+};

+/* not in universal-ctags */ +static TMParserMapEntry powershell_map[] = {

{'f', tm_tag_function_t},

{'v', tm_tag_variable_t},

+};

+typedef struct +{

TMParserMapEntry *entries;

guint size;

+} TMParserMap;

+#define MAP_ENTRY(map) {map, (sizeof(map)/sizeof(TMParserMapEntry))}

could use `G_N_ELEMENTS()`, couldn't it?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/aea7d7845917843dd6b02e5a8dc903...

Colomban Wendling

7:56 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+};

+/* not in universal-ctags */ +static TMParserMapEntry powershell_map[] = {

{'f', tm_tag_function_t},

{'v', tm_tag_variable_t},

+};

+typedef struct +{

TMParserMapEntry *entries;

guint size;

+} TMParserMap;

+#define MAP_ENTRY(map) {map, (sizeof(map)/sizeof(TMParserMapEntry))}

Also, as we now depend on C99, we could have a more safe thing using explicit subobject initialization. Also, if the maps used a streamlined scheme following the parser name (similar to highlightingmappings.h), we could have something even more automated, like i.e.

```c #define MAP_ENTRY_REF(lang, lang_src) [lang] = { map_##lang_src, G_N_ELEMENTS(map_##lang_src) } #define MAP_ENTRY(lang) MAP_ENTRY_REF(lang, lang)

static TMParserMap parser_map[] = { MAP_ENTRY(C), MAP_ENTRY_REF(CPP, C), /* C++ - same as C */ MAP_ENTRY(JAVA), MAP_ENTRY(MAKEFILE), MAP_ENTRY(PASCAL), // ... };

Colomban Wendling

8:20 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+TMTagType tm_parser_get_tag_type(gchar kind, TMParserType lang) +{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->kind == kind)
	return entry->type;
}

return tm_tag_undef_t;
+}

If type lookup happens to be a bottleneck, we could index on kind letter in the maps:

```c static TMTagType map_CONF[128] = { ['n'] = tm_tag_namespace_t, ['m'] = tm_tag_macro_t, }; ``` it obviously has a memory footprint cost though (`sizeof(TMTagType)*128 - (sizeof(TMParserMapEntry - sizeof(TMTagType)) * n_actual_entries` -- likely about 120*4 = 480 bytes per map, so about 24kb), which may or may not be worth it. If we weren't afraid, we could lower this to 52 entries tops instead of 128 (so about 9kb) as we can assume kinds are alphabetic letters, so can do something like that: ```c #define ENTRY(letter, type) \ [(letter >= 'A' && letter <= 'Z') ? (letter - 'A') : \ (letter >= 'a' && letter <= 'z') ? ('Z' + 1 + letter - 'a') : \ -1] = type

static TMTagType map_CONF[26*2] = { ENTRY('n', tm_tag_namespace_t), ENTRY('m', tm_tag_macro_t), }; ```

It could also simply have the kinds sorted alphabetically, too, and use a bsearch.

Anyway, hypothetical problem for now :)

Colomban Wendling

8:26 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+void tm_parser_verify_type_mappings(void) +{
gsize parser_map_size = sizeof(parser_map) / sizeof(TMParserMap);

TMParserType lang;

if (TM_PARSER_COUNT > tm_ctags_get_lang_count())

{
g_warning("More parsers defined in Geany than in ctags");
return;
}

if (parser_map_size != TM_PARSER_COUNT)

{
g_warning("Different number of parsers and tag type mappings");

this one could be a static assertion, would be awesome :) ```c G_STATIC_ASSERT(G_N_ELEMENTS(parser_map == TM_PARSER_COUNT); ```

Colomban Wendling

8:31 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

all the tests should probably be `g_errors()` instead of warnings: if there never are false positive, it should be made impossible to miss.

Colomban Wendling

9:36 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

		if (map->entries[i].kind == kinds[j])

```
			ctags_found = TRUE;
```

		/* check that for every type in ctags there's a type in TM */

		if (map->entries[j].kind == kinds[i])

```
			tm_found = TRUE;
```
```
		if (ctags_found && tm_found)
```
```
			break;
```
```
	}
```
```
	if (!ctags_found)
```

		g_warning("Tag type '%c' found in TM but not in ctags for %s",

			map->entries[i].kind, tm_ctags_get_lang_name(lang));

```
	if (!tm_found)
```

		g_warning("Tag type '%c' found in ctags but not in TM for %s",

			kinds[i], tm_ctags_get_lang_name(lang));

```
	presence_map[map->entries[i].kind]++;
```

```gcc ../../../tagmanager/src/tm_parser.c: In function 'tm_parser_verify_type_mappings': ../../../tagmanager/src/tm_parser.c:637:16: warning: array subscript has type 'char' [-Wchar-subscripts] presence_map[map->entries[i].kind]++; ^ ```

casting to `unsigned char` should be fine.

--- BTW, `presence_map` could be 127 bytes only and indexed as `((unsigned char) map->entries[i].kind) & 0x0f`, kind names won't use non-ASCII letters. But that's probably an over-optimization that might blow off on us someday.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/5d9376ea801a6e6c3ec22748741362...

Colomban Wendling

9:37 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

if ((attrs & tm_tag_attr_access_t) && (TAG_ACCESS_UNKNOWN != tag->access))
fprintf(fp, "%c%c", TA_ACCESS, tag->access);
if ((attrs & tm_tag_attr_impl_t) && (TAG_IMPL_UNKNOWN != tag->impl))
fprintf(fp, "%c%c", TA_IMPL, tag->impl);
if (fprintf(fp, "\n"))
return TRUE;
else
return FALSE;
+}

+GPtrArray *tm_source_file_read_tags_file(const gchar *tags_file, TMParserType mode) +{

guchar buf[BUFSIZ];

FILE *fp;

GPtrArray *file_tags, *new_tags;

```gcc ../../../tagmanager/src/tm_source_file.c: In function 'tm_source_file_read_tags_file': ../../../tagmanager/src/tm_source_file.c:527:25: warning: unused variable 'new_tags' [-Wunused-variable] GPtrArray *file_tags, *new_tags; ^ ```

Colomban Wendling

9:40 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

const gchar *file_name, TMParserType lang, tm_ctags_callback callback,

gpointer user_data);

+const gchar *tm_ctags_get_lang_name(TMParserType lang);

+TMParserType tm_ctags_get_named_lang(const gchar *name);

+const gchar *tm_ctags_get_lang_kinds(TMParserType lang);

+const gchar *tm_ctags_get_kind_name(gchar kind, TMParserType lang);

+gchar tm_ctags_get_kind_from_name(const gchar *name, TMParserType lang);

+gboolean tm_ctags_is_using_regex_parser(TMParserType lang);

+guint tm_ctags_get_lang_count();

`(void)`

Colomban Wendling

9:40 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

+void tm_ctags_init();

`(void)`

Colomban Wendling

9:42 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -45,6 +45,8 @@ typedef struct TMSourceFile

GType tm_source_file_get_type(void);

+void tm_source_file_ctags_init();

that should also be `GEANY_PRIVATE`, shouldn't it?

Colomban Wendling

4 May 4 May

12:08 a.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

and this should be run during `make check` instead of when running Geany itself. I've got a working version of this, will clean it up and show it soon (hopefully tomorrow).

Jiří Techet

10:35 a.m.

New subject: [geany/geany] TM cleanups (#957)

@b4n Thanks a lot for all the comments! Will go through them later today.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-216824455

Jiří Techet

10:49 a.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Forgot to mention - the check is really cheap. I tested it both on my normal machine where for all the languages the check took 10-20 microseconds and on Raspberry 3 it took 100-200 microseconds and even if we add more languages and types, it will probably always be below 1ms and completely invisible to user.

Jiří Techet

11:59 a.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag)

...

Mmmwell, this is pretty specific to the Python parser and not well-contained, but well, probably still better than a whole new hook indeed.

I don't find it a big problem - Geany is full of if (language) {do stuff} things and this is just one more (and IMO much cleaner than the direct parser->TM callback).

...

BTW, this might be fixable in the parser side using the U-CTags' new Cork thingie? Not sure and not tested, but that would be nice.

I haven't checked what the cork thing in uctags does so not sure either.

Jiří Techet

noon

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag) +{
guint i;

const char *parent_tag_name;

if (tag->type != tm_tag_method_t || tag->scope == NULL ||
g_strcmp0(tag->name, "__init__") != 0)
return;
parent_tag_name = strrchr(tag->scope, '.');

Yep, it seemed unused so I dropped it.

elextr

12:07 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag)

Geany will become un-maintainable if its filled with `if(language){do stuff }` sections. The attention should be removing them not adding "just one more".

Jiří Techet

12:14 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

...

This is an odd type name for TM/Geany, we generally use CamelCase for those -- TMCtagsCallback or something?

OK.

...

It also seems to have a strange signature only targeted at our very specific usage, although it suggest it's meant to be a generic API. By that I mean the invalidate parameter, that seem more like a step indicator that might be better served as an enum, or at least rather name it something like "initialize" or something more generic...

This isn't meant to be some final API and it's indeed targeted to Geany's specific uses. The thing about "invalidate" and multi-pass parsing is that I think it would be best to get rid of it completely and use just a single pass. From the API perspective it's really stupid because this means the API cannot be used as a stream - you cannot receive a tag, process it somehow and pass it somewhere (e.g. send over network) - you have to wait until the parsing is finished and only then you can be sure the tags are valid.

By the way, I tried the c.c parser on Geany+glib+glibc, then linux kernel and also the boost library sources and the second pass was never executed (plus all the Geany unit tests pass without it). I'm sure it can be useful in some very specific cases but I'm also sure one could introduce passes 2, 3, 4, 5... for other specific cases and this really isn't the way to go. So if there is some real ctags API in the future, IMO the way to go is to remove the "invalidate" parameter and multi-pass parsing.

Jiří Techet

12:19 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -72,6 +72,9 @@ static gboolean tm_create_workspace(void) theWorkspace->source_files = g_ptr_array_new(); theWorkspace->typename_array = g_ptr_array_new(); theWorkspace->global_typename_array = g_ptr_array_new();

tm_source_file_ctags_init();

Yes, will fix. Originally I didn't have the ctags wrapper file and though all the ctags code would be handled by tm_source_file and this remained.

elextr

12:31 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

I vaguely remember having worked out once that multi-passes was an attempt to un-confuse itself if it got mixed up due to braces embedded in literal strings inside function bodies (or other constructs it doesn't properly parse).

Jiří Techet

12:31 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -128,7 +128,8 @@ extern void checkRegex (void);

/* Extra stuff for Tag Manager */ extern tagEntryFunction TagEntryFunction; -extern void setTagEntryFunction(tagEntryFunction entry_function); +extern void *TagEntryUserData;

Yeah, agree, just for now I tried to make minimal changes to ctags as I want to sync them with uctags anyway and do the "right" stuff once the sync is done.

Jiří Techet

12:42 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+};

+/* not in universal-ctags */ +static TMParserMapEntry powershell_map[] = {

{'f', tm_tag_function_t},

{'v', tm_tag_variable_t},

+};

+typedef struct +{

TMParserMapEntry *entries;

guint size;

+} TMParserMap;

+#define MAP_ENTRY(map) {map, (sizeof(map)/sizeof(TMParserMapEntry))}

After posting the patch I realized it would be probably nicer instead of doing

``` MAP_ENTRY(c_map), /* C++ - same as C */ ```

to define

``` static TMParserMapEntry cpp_map[] = c_map; ```

and then just use MAP_ENTRY(cpp_map) in the parser_map definition. Could do the macro stuff you suggest too, just to me it usually takes more time to decode what macros with ## do than not using such features ;-).

Jiří Techet

12:45 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+TMTagType tm_parser_get_tag_type(gchar kind, TMParserType lang) +{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->kind == kind)
	return entry->type;
}

return tm_tag_undef_t;
+}

I don't think the lookup will be a bottleneck - compared to reading characters during parsing, tags are generated quite rarely and even with the current TMTag generation where the whole tag type strings are compared, I've never seen this in the profile. So I'd skip this for now.

Jiří Techet

12:47 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

		if (map->entries[i].kind == kinds[j])

```
			ctags_found = TRUE;
```

		/* check that for every type in ctags there's a type in TM */

		if (map->entries[j].kind == kinds[i])

```
			tm_found = TRUE;
```
```
		if (ctags_found && tm_found)
```
```
			break;
```
```
	}
```
```
	if (!ctags_found)
```

		g_warning("Tag type '%c' found in TM but not in ctags for %s",

			map->entries[i].kind, tm_ctags_get_lang_name(lang));

```
	if (!tm_found)
```

		g_warning("Tag type '%c' found in ctags but not in TM for %s",

			kinds[i], tm_ctags_get_lang_name(lang));

```
	presence_map[map->entries[i].kind]++;
```

Yeah, I think this doesn't matter performance/memory-wise.

Jiří Techet

12:57 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag)

But if you have a look how it's done now, it's much worse and much more unmaintainable than this - it introduces diff to the python parser compared to universal-ctags and worse, it makes a very strange direct path python_parser->TM (no other parser does this, all of them generate tags the standard way).

Such a strange callback would definitely not become part of ctags API if it's ever turned into a library so we'd have to maintain our own version. The "just one more" is very well justified in this case IMO (other option is not to add the constructor parameters to the class type and drop this feature).

Jiří Techet

12:59 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

Yes, it looks like something like this but apparently it doesn't happen very often, at least judging from the projects I tested it on so the question is whether it's worth it.

elextr

1:03 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag)

Then how does u-ctags do it?

Jiří Techet

1:29 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

@@ -109,6 +109,35 @@ gchar *tm_get_real_path(const gchar *file_name) return NULL; }

+/* add argument list of __init__() Python methods to the class tag */ +static void update_python_arglist(const TMTag *tag)

uctags doesn't do (and in fact cannot do) anything - it was added by Enrico in 1932d3fae938a21c1a0b22c4ae205f63f8ff8d0c. The purpose of it is for:

``` class Foo: def __init__(self, bar, baz): pass ```

to use the tag for its __init__() and copy its argument list to the tag of Foo so when you type

``` a = Foo( ```

you get completion based on the Foo's __init__(). Thing like that cannot be done in uctags because at the time __init__() is parsed, the Foo tag is already written to disk and cannot be modified to include the arguments of __init__().

Colomban Wendling

3:58 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

I don't really worry about checks taking up time, I rather worry about issues not getting noticed because they are a mere runtime warning. Having them run on `make check` and error-out will make Travis and `make dist` fail if they fail, so they should never get unnoticed.

Colomban Wendling

4:41 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

AFAIK it's meant to recover from weird proeproc cases, i.e. imagine this C code: ```c #define FOO 0

int main(void) { #if FOO if (1) { #endif // ... #if ! FOO // ... #else } #endif

return 0; }

int foo() {

} ```

It has unmatched braces if you always take the first preproc branch.

However, AFAICT the current code can *never* hit: the `passCount` starts at 0 (I blamed it back to the initial revision), but the only code path setting `retry` to `TRUE` is guarded with `if (exception == ExceptionBraceFormattingError && passCount == 1)`, so it could only happen on the *second* pass, which can't itself won't happen as it won't ever retry. I imagine this is an oversight at some point and `passCount` shouldstart at 1, but indeed currently it seems like plain dead code.

U-Ctags doesn not have this bug, because it indeed starts passCount at one, by incrementing it before calling the rescanning parser: `createTagsForFile (fileName, language, ++passCount)`, so it does start at 1, not 0. With current version of the OldC parser (somewhat the one we have) you'll see it does output the `foo` tag we don't: ```terminal $ ./ctags --verbose --languages=OldC -o - file.c ... OPENING file.c as OldC language file /tmp/a.c: failed to find match for '{' at line 5 /tmp/a.c: retrying file with fallback brace matching algorithm OPENING file.c as OldC language file sorting tag file system ("sort -u") FOO file.c /^#define FOO /;" d file: foo file.c /^int foo() {$/;" f main file.c /^int main(void)$/;" f ```

Colomban Wendling

5:19 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+};

+/* not in universal-ctags */ +static TMParserMapEntry powershell_map[] = {

{'f', tm_tag_function_t},

{'v', tm_tag_variable_t},

+};

+typedef struct +{

TMParserMapEntry *entries;

guint size;

+} TMParserMap;

+#define MAP_ENTRY(map) {map, (sizeof(map)/sizeof(TMParserMapEntry))}

...

Could do the macro stuff you suggest too, just to me it usually takes more time to decode what macros with ## do than not using such features ;-).

It sure hides some logic, but OTOH it can avoid typos in repetitive code, as well as making it easier to change it. In this case, instead of doing `[TM_PARSER_C] = { map_C, G_N_ELEMENTS(map_C) }` we get `MAP_ENTRY(C)`, which, IMO, is worth it, esp. as it's very repetitive.

BTW, I see I made a typo in my previous post's macro, missing the `TM_PARSER_##` prefix to `lang`.

...

After posting the patch I realized it would be probably nicer instead of doing […]

Indeed. Can even be a simple `#define map_CPP map_C` avoiding any actual data duplication.

Jiří Techet

5:23 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Yes, I agree with the g_error() stuff - I was referring to the patch you are working on and whether it's necessary (I guess you want to have some command-line flag to enable-disable the test, right?). Without any such patch the check will be done every time geany starts so totally about 300 times when run for every test - still the total time spent on the checks will be less than 1ms on my machine and similarly marginal compared to the total time spent on the tests for every other platform. So the question is whether such a patch is worth the work.

Colomban Wendling

5:31 p.m.

New subject: [geany/geany] TM cleanups (#957)

Some follow-up stuff at https://github.com/b4n/geany/commits/ctags_hook%2Bfollowup_ (not necessarily good to take as is, but may or may not be worth integrating)

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-216941038

Colomban Wendling

5:33 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

I was thinking about a separate test executable linked to *libtagmanager.la* and doing just that: https://github.com/b4n/geany/commit/2453bc853bf4716ba41601fe037337ef9cc582f8 If this doesn't lead to any building issues (I need to check if linking a `.la` to both an executable and a library doesn't require building each file both as relocatable and not on some archs, which would blow compile times off the roof, like in early libtool switch), it's fairly trivial and self-contained I think.

Jiří Techet

6:16 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

I'm just wondering whether all the return value propagation is necessary - if we call g_error() instead of g_warning() everywhere, it should guarantee the process terminates with a non-0 exit code and the patch shouldn't be necessary. Or do I miss something?

And thanks a lot for all the patches - I went through them quickly and they look great.

Colomban Wendling

6:23 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Nah, I propagated the errors just because it allows to have all the warnings at once, but it's probably not worth it, and using `g_error()` or `g_assert()` would be just fine and simpler indeed.

Jiří Techet

6:32 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

And one more thing - I was thinking about moving the TM part into Geany directory (probably under a subdirectory such as "tm") because it really is a Geany code and then renaming the "tagmanager" directory to "ctags" and making the directory structure match uctags. For the moved TM part it would be nice to have access to utils.c/h to make the code match Geany's style better using some of the utils and to have access to some Geany settings (e.g. https://github.com/geany/geany/pull/963/commits/9c16401c94d6de324854c06312ec... would need it). Then TM would depend on Geany and it wouldn't be possible to create a Geany-independent binary.

elextr

11:24 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Perhaps it would be better to split ctags parsers into two directories, those that are from upstream and those that are Geany specific. Would make updating from upstream easier and less risky.

Jiří Techet

5 May 5 May

12:03 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

At the moment there are no parsers that would be completely identical. For Geany 1.29 I'd like to start syncing the implementations with uctags and it will be easier to make a diff of the whole parser directory against uctags to see:

1. Which parsers are in uctags and not in Geany. 2. Which parsers are in Geany and not in uctags. 3. How the implementations differ.

And this will be easier with a single parser directory.

elextr

12:07 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Yeah, they all start in the "unsynced" directory and move to the other as they get accepted into upstream.

Predict c.c is last :wink:

Jiří Techet

1:08 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

...

Yeah, they all start in the "unsynced" directory and move to the other as they get accepted into upstream.

Yeah, I could introduce something like that once something gets actually synced - now the directory would be just empty.

...

Predict c.c is last :wink:

Yeah, I think you are good at predicting future :-).

There's by the way a new cxx parser in uctags (see the cxx directory) which, if I understand it correctly, parses also function bodies and which might fix many issues of the current c.c parser, especially for C++. It's used by default by uctags now - you could give it a try if it fixes some of the issues you reported. If it works better, it might be interesting for us to switch to it.

Jiří Techet

6 May 6 May

2:40 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

*/

+#ifndef TM_CTAGS_WRAPPERS +#define TM_CTAGS_WRAPPERS

+#include <glib.h>

+#include "tm_parser.h"

+#include "entry.h" /* for sTagEntryInfo */

+G_BEGIN_DECLS

+typedef gboolean (*tm_ctags_callback) (const tagEntryInfo *const tag,

gboolean invalidate, void *user_data);

OK I tried setting passCount=1 at the beginning and there are indeed a few (but really few) cases in the testing projects where the second pass is used. Actually maybe it's OK to leave the multiple passes - users of the library may just decide to ignore subsequent passes. By the way, I made the callback return boolean which is unused now but which in the future might serve as an indicator whether parsing should stop.

Maybe cleaner way than the current boolean flag or your enum would be having multiple callbacks fired at specific occasions. Two should be enough I think - the finalize one shouldn't be necessary because the caller knows when parsing finishes (it's when tm_ctags_parse() returns) and there's no need for it between passes because it would just be called immediately before the initialize step.

Jiří Techet

8:14 p.m.

New subject: [geany/geany] TM cleanups (#957)

@b4n I pushed your patches together with other fixes based on your comments - I hope I didn't forget anything. From your patches I just dropped https://github.com/b4n/geany/commit/2453bc853bf4716ba41601fe037337ef9cc582f8 as using g_error() seems to be simpler and also the separate binary would slightly conflict with the source file reorganization I'd like to make.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-217546922

Jiří Techet

8 Jun 8 Jun

11:58 a.m.

New subject: [geany/geany] TM cleanups (#957)

@b4n Any chance to get this merged? At the beginning of the next release cycle I'd like to start syncing the ctags implementations and this has to be applied before.

Also for 1.28 I'd like to do the TM source move-around so it matches the uctags directory structure. It should be quite low-risk as no real sources will be modified, mostly just makefiles so I hope it won't be a problem for 1.28. But again this requires these patches are in.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-224567905

Colomban Wendling

4:26 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+}

+gchar tm_parser_get_tag_kind(TMTagType type, TMParserType lang) +{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';

shouldn't this rather be something like `0`?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957/files/6e89ee5ebef888e5d07407d33ffb4d...

Colomban Wendling

4:32 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

...

And one more thing - I was thinking about moving the TM part into Geany directory […]. Then TM would depend on Geany and it wouldn't be possible to create a Geany-independent binary.

Should still be possible to have a separate binary for checking, so long as the symbol is public. We could have either that function public, or a `geany_test_main()` that calls a bunch of tests and can be run without a GUI. Or something like @codebrainz's idea of something like a self-test option, `--test` that would perform tests and exit (with out without GUI, one could probably be emulated somehow with some weird X-faking thingie).

Anyway, can be done later, but really having tests run with the test suite would be great.

Colomban Wendling

4:43 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

includes_files = g_list_reverse (includes_files);

return g_list_reverse(includes_files);

nothing new in the patch set, but reversing the list here is most probably absurd, as the order in the hash table isn't guaranteed, and unlikely to be neither alphabetical nor in order.

Colomban Wendling

4:48 p.m.

New subject: [geany/geany] TM cleanups (#957)

I'll work on it.

BTW, we'll most probably need a way to enable/disable CTags kinds at some point if we want to use stock CTags parsers. Using `tm_tag_undef_t` is cute, but it's (slightly) less efficient, and can't enabled a disabled kind. Should be possible somehow, as CTags does have CLI flags to do just that.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-224653569

Jiří Techet

9 Jun 9 Jun

8:35 a.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

...

Should still be possible to have a separate binary for checking, so long as the symbol is public. We could have either that function public, or a geany_test_main() that calls a bunch of tests and can be run without a GUI. Or something like @codebrainz's idea of something like a self-test option, --test that would perform tests and exit (with out without GUI, one could probably be emulated somehow with some weird X-faking thingie).

What's the point of the separate testing binary? Or in other words, what's wrong with the current way the tests are run (already without GUI)? I don't understand what this is trying to achieve.

Colomban Wendling

10 Jun 10 Jun

9:01 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

The point would be being able to run something as part of `make check`.

Colomban Wendling

9:57 p.m.

New subject: [geany/geany] TM cleanups (#957)

Merged #957.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#event-689161643

Colomban Wendling

10:36 p.m.

New subject: [geany/geany] TM cleanups (#957)

Oops, `tm_parser_verify_type_mappings()` is not called anymore at all (4036d7d18a22af98beee1644d393e73240e3aee5 removed it), meaning the testing has been dropped -- it used to have moved to a test binary, but that one wasn't imported here.

I guess I should reintroduce a call to that in `tm_create_workspace()`, @techee ?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-225312211

Colomban Wendling

10:39 p.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Hum, actually if the test was properly made when intializing CTags, current test suite would catch it as it runs Geany on the tags file, so I guess it's fine (just need to fix so the test is actually run)

Colomban Wendling

11:05 p.m.

New subject: [geany/geany] TM cleanups (#957)

#1068

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/957#issuecomment-225316265

Jiří Techet

11 Jun 11 Jun

8:24 a.m.

New subject: [geany/geany] TM cleanups (#957)

...

+{
TMParserMap *map = &parser_map[lang];

guint i;

for (i = 0; i < map->size; i++)

{
TMParserMapEntry *entry = &map->entries[i];
if (entry->type == type)
	return entry->kind;
}

return '-';
+}

+void tm_parser_verify_type_mappings(void)

Yeah, exactly, the test is made already with the current way of "make check".

And thanks for all the work reviewing this patch set! (Hopefully the last of the hard-to-review ones.)

3051

Age (days ago)

3144

Last active (days ago)

github-comments@lists.geany.org

76 comments

3 participants

tags (0)

participants (3)

Colomban Wendling
elextr
Jiří Techet