Don't use temporary file when creating tag files without running preprocessor #3204

techee · 2022-05-13T17:49:47Z

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.

elextr · 2022-05-15T00:10:54Z

src/tagmanager/tm_workspace.c

@@ -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.

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).

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.

elextr · 2022-05-15T00:11:18Z

src/tagmanager/tm_workspace.c

 			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.

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)

elextr · 2022-05-15T00:33:11Z

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 · 2022-05-15T21:25:36Z

OK so if I understand the conclusion of #3205 correctly, is it OK to remove the globbing feature completely?

elextr · 2022-05-15T23:33:23Z

Not as clear cut maybe 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- · 2022-07-11T06:17:16Z

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?

The code was probably originally written for c/c++ only and all the mentions of "include files" mean "source files" for other languages.

techee · 2022-08-28T19:55:56Z

Is it OK to force-push the patches rebased on top of master (to resolve conflict with already merged #3207)?

kugel- · 2022-08-28T20:02:34Z

Yes, see the comment above :-)

techee · 2022-08-28T20:10:58Z

OK, done.

kugel-

Good stuff, only minor comments that you can chose to ignore.

kugel- · 2022-08-28T20:14:36Z

src/tagmanager/tm_workspace.c

+				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()

I still tend to follow the other code's style despite Geany now uses C99.

kugel- · 2022-08-28T20:14:54Z

src/tagmanager/tm_workspace.c

+		GPtrArray *tags = g_ptr_array_new();
+		GSList *tm_source_files = NULL;
+
+		for (node = source_files; node; node = node->next)


Could use foreach_list()

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.

Oh, it's under src/ so I consider it part of Geany completely. But it's "your area" so OK

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.

kugel- · 2022-08-28T20:16:15Z

src/tagmanager/tm_workspace.c

+		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?

Yeah, that's probably better. Will do.

kugel-

Still approving. I suggest to squash-merge the end result.

…rocessor The tm_workspace_create_global_tags() function generates tags for two cases: 1. Source files pre-processed by C pre-processor. It first generates a file combining the parsed header files by creating a temporary file in which all the header files are included and this file is passed to the pre-processor. The result of the pre-processed file is then parsed by the ctags parser. 2. Source files directly parsed by the ctags parser. In this case all the source files are concatenated to a single file which is then parsed by the ctags parser. This patch leaves (1) more or less unchanged; however, the creation of the temporary file in (2) is unnecessary - the individual files can be parsed directly, the tags from all the parses can be combined, sorted and pruned without creating the temporary file. The temporary file is a problem for unit tests where some languages use the file name as the name of module in which the tags are defined and by using a different name, the unit test generates a different tag file every time it's run. Note the changed output of the process_order unit test caused by this change. The test parses two files, one containing enum { I1_E1, I1_E2, }; the other contining enum { I2_E1, I2_E2, }; Previously, because the files were concatenated the enums were different tags and the anonnymous tag renaming function renamed them to anon_enum_1 and anon_enum_2. Because now the files are parsed separately, the enum from the first file gets renamed to anon_enum_1 and when parsing the second file, we get the name anon_enum_1 as well for the second enum. This however isn't a problem - we don't display global tags in the symbol tree, autocompletion, and they are also ignored in scope completion so this shouldn't matter much.

techee · 2022-08-28T21:29:23Z

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.

techee added the tag-manager Tag manager related patches and bugs label May 13, 2022

techee mentioned this pull request May 13, 2022

Add Ocaml ctags parser #3163

Merged

elextr reviewed May 15, 2022

View reviewed changes

elextr mentioned this pull request May 15, 2022

Problem with globbing on generating Geany global tags #3205

Closed

techee mentioned this pull request May 17, 2022

Remove built in globbing support when generating tag files #3207

Merged

Rename variables in tag file writing functions to make things clearer

30f5514

The code was probably originally written for c/c++ only and all the mentions of "include files" mean "source files" for other languages.

techee force-pushed the separate_parsing branch from f5c7f6f to 8a40f2c Compare August 28, 2022 20:03

kugel- approved these changes Aug 28, 2022

View reviewed changes

techee force-pushed the separate_parsing branch from 3ad0f50 to 8c7081c Compare August 28, 2022 21:22

techee merged commit 2884b02 into geany:master Sep 4, 2022

b4n added this to the 1.39/2.0 milestone Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use temporary file when creating tag files without running preprocessor #3204

Don't use temporary file when creating tag files without running preprocessor #3204

techee commented May 13, 2022

elextr May 15, 2022

techee May 15, 2022

kugel- Jul 11, 2022

elextr May 15, 2022

kugel- Jul 11, 2022

elextr commented May 15, 2022

techee commented May 15, 2022

elextr commented May 15, 2022

kugel- commented Jul 11, 2022

techee commented Aug 28, 2022

kugel- commented Aug 28, 2022

techee commented Aug 28, 2022

kugel- left a comment

kugel- Aug 28, 2022

techee Aug 28, 2022

kugel- Aug 28, 2022

techee Aug 28, 2022

kugel- Aug 28, 2022

techee Aug 28, 2022

kugel- Aug 28, 2022

techee Aug 28, 2022

kugel- left a comment •

edited

techee commented Aug 28, 2022

Don't use temporary file when creating tag files without running preprocessor #3204

Don't use temporary file when creating tag files without running preprocessor #3204

Conversation

techee commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elextr commented May 15, 2022

techee commented May 15, 2022

elextr commented May 15, 2022

kugel- commented Jul 11, 2022

techee commented Aug 28, 2022

kugel- commented Aug 28, 2022

techee commented Aug 28, 2022

kugel- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kugel- left a comment • edited

Choose a reason for hiding this comment

techee commented Aug 28, 2022

kugel- left a comment •

edited