This patch avoids creating a temporary file containing concatenated source files passed on the command-line when the `-P` option is used (or when tags are generated for other languages than C/C++) by e.g. ``` geany -P -g file1.h file2.h file3.h ``` More discussion can be found in #3163. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3204
-- Commit Summary --
* Rename variables in tag file writing functions to make things clearer * Don't use temporary file when creating tag files without running preprocessor
-- File Changes --
M src/tagmanager/tm_workspace.c (161) M tests/ctags/process_order.c.tags (5)
-- Patch Links --
https://github.com/geany/geany/pull/3204.patch https://github.com/geany/geany/pull/3204.diff
@elextr commented on this pull request.
@@ -453,19 +419,19 @@ static GList *lookup_includes(const gchar **includes, gint includes_count)
#ifdef HAVE_GLOB_H globbuf.gl_offs = 0;
- if (includes[0][0] == '"') /* leading " char for glob matching */ + if (sources[0][0] == '"') /* leading " char for glob matching */
I know its not something you changed (except the variable name) but where did this `"` come from? Its not mentioned in the manual as being required for globbing, nor in the parameter description of `sources`?
Also its only tested on `sources[0]` but then assumed on all sources below.
@elextr commented on this pull request.
gchar *clean_path;
if (dirty_len < 2) continue;
clean_path = g_malloc(dirty_len - 1);
- strncpy(clean_path, includes[i] + 1, dirty_len - 1); + strncpy(clean_path, sources[i] + 1, dirty_len - 1);
Skips first character of source, see above.
I know its not something this PR changes, but fixing globbing it will probably conflict with this PR, so maybe it needs to be fixed first, created #3205?
@techee commented on this pull request.
@@ -453,19 +419,19 @@ static GList *lookup_includes(const gchar **includes, gint includes_count)
#ifdef HAVE_GLOB_H globbuf.gl_offs = 0;
- if (includes[0][0] == '"') /* leading " char for glob matching */ + if (sources[0][0] == '"') /* leading " char for glob matching */
I know its not something you changed (except the variable name) but where did this " come from? Its not mentioned in the manual as being required for globbing, nor in the parameter description of sources?
This seems to be wrong. I think this was meant to detect ``` geany -g * ``` vs ``` geany -g "*" ``` where in the first case the shell expands `*` to the list of files Geany receives as its argument list while in the second case it receives `*` (but without the `"` which gets removed by the shell).
Not sure what we want and whether we actually want/need to support globbing by ourselves or whether what the shell does for us is sufficient already. I think the situation where this might be useful is when there are too many files matching the patterns so they exceed the max. number of command-line arguments.
Maybe we could just drop this check completely and always try to apply globbing (when `HAVE_GLOB_H` is defined).
OK so if I understand the conclusion of #3205 correctly, is it OK to remove the globbing feature completely?
Not as clear cut [maybe](https://github.com/geany/geany/issues/3205#issuecomment-1127085617) but whichever decision made on #3205 its not part of this PR.
As it stands this PR LGTM on Linux with it doing the globbing.
@kugel- commented on this pull request.
@@ -453,19 +419,19 @@ static GList *lookup_includes(const gchar **includes, gint includes_count)
#ifdef HAVE_GLOB_H globbuf.gl_offs = 0;
- if (includes[0][0] == '"') /* leading " char for glob matching */ + if (sources[0][0] == '"') /* leading " char for glob matching */
My preference (in that order) 1 Leave globbing to the shell 2 explicitly enable internal globbing with a separate parameter 3 Always apply globbing
I'm not sure if 1 leaves windows users behind, but if not I would greatly prefer that.
@kugel- commented on this pull request.
gchar *clean_path;
if (dirty_len < 2) continue;
clean_path = g_malloc(dirty_len - 1);
- strncpy(clean_path, includes[i] + 1, dirty_len - 1); + strncpy(clean_path, sources[i] + 1, dirty_len - 1);
Old code so I would ignore this problem for now.
But IIUC this is only a problem for the second and later arguments as the first should be enclosed by `"` (so `clean_path` strips `"`), right?
Plus, as @techee mentions, the condition is probably never true. Best to leave globbing to the shell really (or clever find+xargs tricks)
Heh, should read the comments completely before looking at the code. So this needs a rebase and then we're back in the green, right?
Is it OK to force-push the patches rebased on top of master (to resolve conflict with already merged #3207)?
Yes, see the comment above :-)
@techee pushed 2 commits.
30f5514465e1e98383cff9dc7e88627f13aafd41 Rename variables in tag file writing functions to make things clearer 8a40f2ce1e208db484ee6d58551ad6a2b559f90f Don't use temporary file when creating tag files without running preprocessor
OK, done.
@kugel- approved this pull request.
Good stuff, only minor comments that you can chose to ignore.
- else
+ { + GList *node; + GPtrArray *filtered_tags; + GPtrArray *tags = g_ptr_array_new(); + GSList *tm_source_files = NULL; + + for (node = source_files; node; node = node->next) + { + TMSourceFile *source_file = tm_source_file_new(node->data, tm_source_file_get_lang_name(lang)); + if (source_file) + { + guint i; + tm_source_files = g_slist_prepend(tm_source_files, source_file); + tm_source_file_parse(source_file, NULL, 0, FALSE); + for (i = 0; i < source_file->tags_array->len; i++)
Could declare `guint i` inside the `for`, or alternatively use our beloved `foreach_array()`
+gboolean tm_workspace_create_global_tags(const char *pre_process_cmd, const char **sources,
+ int sources_count, const char *tags_file, TMParserType lang) +{ + gboolean ret = FALSE; + GList *source_files = lookup_sources(sources, sources_count); + + if (pre_process_cmd) + ret = create_global_tags_preprocessed(pre_process_cmd, source_files, tags_file, lang); + else + { + GList *node; + GPtrArray *filtered_tags; + GPtrArray *tags = g_ptr_array_new(); + GSList *tm_source_files = NULL; + + for (node = source_files; node; node = node->next)
Could use `foreach_list()`
- are allowed.
+ @param tags_file The file where the tags will be stored. + @param lang The language to use for the tags file. + @return TRUE on success, FALSE on failure. +*/ +gboolean tm_workspace_create_global_tags(const char *pre_process_cmd, const char **sources, + int sources_count, const char *tags_file, TMParserType lang) +{ + gboolean ret = FALSE; + GList *source_files = lookup_sources(sources, sources_count); + + if (pre_process_cmd) + ret = create_global_tags_preprocessed(pre_process_cmd, source_files, tags_file, lang); + else + { + GList *node;
Maybe stuff the else case also in a static function like the preprocess case?
@techee commented on this pull request.
+gboolean tm_workspace_create_global_tags(const char *pre_process_cmd, const char **sources,
+ int sources_count, const char *tags_file, TMParserType lang) +{ + gboolean ret = FALSE; + GList *source_files = lookup_sources(sources, sources_count); + + if (pre_process_cmd) + ret = create_global_tags_preprocessed(pre_process_cmd, source_files, tags_file, lang); + else + { + GList *node; + GPtrArray *filtered_tags; + GPtrArray *tags = g_ptr_array_new(); + GSList *tm_source_files = NULL; + + for (node = source_files; node; node = node->next)
`foreach_*()` and other Geany stuff isn't currently available in tag manager - it's still in the form of (kind of) external library and doesn't include anything from Geany.
@techee commented on this pull request.
- else
+ { + GList *node; + GPtrArray *filtered_tags; + GPtrArray *tags = g_ptr_array_new(); + GSList *tm_source_files = NULL; + + for (node = source_files; node; node = node->next) + { + TMSourceFile *source_file = tm_source_file_new(node->data, tm_source_file_get_lang_name(lang)); + if (source_file) + { + guint i; + tm_source_files = g_slist_prepend(tm_source_files, source_file); + tm_source_file_parse(source_file, NULL, 0, FALSE); + for (i = 0; i < source_file->tags_array->len; i++)
I still tend to follow the other code's style despite Geany now uses C99.
@techee commented on this pull request.
- are allowed.
+ @param tags_file The file where the tags will be stored. + @param lang The language to use for the tags file. + @return TRUE on success, FALSE on failure. +*/ +gboolean tm_workspace_create_global_tags(const char *pre_process_cmd, const char **sources, + int sources_count, const char *tags_file, TMParserType lang) +{ + gboolean ret = FALSE; + GList *source_files = lookup_sources(sources, sources_count); + + if (pre_process_cmd) + ret = create_global_tags_preprocessed(pre_process_cmd, source_files, tags_file, lang); + else + { + GList *node;
Yeah, that's probably better. Will do.
@techee pushed 1 commit.
3ad0f508c36be2a93210aa9ec077d5b639485616 Move global tag file creation without preprocessing to separate function
@kugel- commented on this pull request.
+gboolean tm_workspace_create_global_tags(const char *pre_process_cmd, const char **sources,
+ int sources_count, const char *tags_file, TMParserType lang) +{ + gboolean ret = FALSE; + GList *source_files = lookup_sources(sources, sources_count); + + if (pre_process_cmd) + ret = create_global_tags_preprocessed(pre_process_cmd, source_files, tags_file, lang); + else + { + GList *node; + GPtrArray *filtered_tags; + GPtrArray *tags = g_ptr_array_new(); + GSList *tm_source_files = NULL; + + for (node = source_files; node; node = node->next)
Oh, it's under src/ so I consider it part of Geany completely. But it's "your area" so OK
@kugel- approved this pull request.
Still approving. I suggest squash-merge the end result-
@techee commented on this pull request.
+gboolean tm_workspace_create_global_tags(const char *pre_process_cmd, const char **sources,
+ int sources_count, const char *tags_file, TMParserType lang) +{ + gboolean ret = FALSE; + GList *source_files = lookup_sources(sources, sources_count); + + if (pre_process_cmd) + ret = create_global_tags_preprocessed(pre_process_cmd, source_files, tags_file, lang); + else + { + GList *node; + GPtrArray *filtered_tags; + GPtrArray *tags = g_ptr_array_new(); + GSList *tm_source_files = NULL; + + for (node = source_files; node; node = node->next)
It's not that I'm against it, it's just that nobody did it yet. If we decide to do that, we should probably do it properly and go through all the `for` loops and replace them if possible plus do other geany-style changes.
I've pushed the squashed version. I'll keep this PR open for about a week and unless there are objections from others, I'll merge it then.
Merged #3204 into master.
github-comments@lists.geany.org