Don't use the files inode as the hash. Although it looks like a good idea for de-duplicating links as well, it has several issues, including non-uniqueness of inodes across file systems. The way it was done hashing the inode but comparing the file name string pointers also made the hash mostly irrelevant, as it just stored filenames sharing the same inode in the same hash bucket but without actually doing any de-duplication, making the whole thing a convoluted way of converting to a list.
Instead, hash and compare the filenames themselves, which, even though it doesn't handle links de-duplication, is better than the non-functional previous code.
Also, directly build the list and only use the hash table as a way for checking for duplicates, which is both faster and gives a stable output.
See #1989 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1991
-- Commit Summary --
* tm: Cleanup include lookup
-- File Changes --
M src/tagmanager/tm_workspace.c (18)
-- Patch Links --
https://github.com/geany/geany/pull/1991.patch https://github.com/geany/geany/pull/1991.diff
elextr commented on this pull request.
@@ -515,6 +504,7 @@ static GList *lookup_includes(const gchar **includes, gint includes_count)
{ gchar *file_name_copy = g_strdup(globbuf.gl_pathv[idx_glob]);
+ include_files = g_list_prepend(include_files, file_name_copy);
includes_files (see Travis)
elextr commented on this pull request.
@@ -535,12 +525,12 @@ static GList *lookup_includes(const gchar **includes, gint includes_count)
{ gchar* file_name_copy = g_strdup(includes[i]);
+ include_files = g_list_prepend(include_files, file_name_copy);
as above
The whole `tm_file_inode_hash` function can also be dropped, because it is unused
Oops, that's what you get for making changes and committing in a hurry. Fixed, and this time I built it before, and ran our tests.
There is 6-9 lines of code duplication around the `g_list_prepend` calls - could be improved in a later PR.
@b4n pushed 2 commits.
939dab0 Process files in the order they are listed when generating a tags file 998760c Add a test for the processing order when generating a tags file
@bmwiedemann what do you mean, between the glob and non-glob versions? I don't really mind given how the glob conditional is not trivial.
I just added an extra 2 things here: 1. process files in the order they appear on the command line. Before, the order was stable for a given command line, but was reversed, which IMO is clearly not the expected behavior. This said, basically nobody should care. 2. I added a test case that verifies the processing order.
LGBI
Is the runner.sh change really part of this, or is it a general change that possibly should be separate?
Is the runner.sh change really part of this, or is it a general change that possibly should be separate?
Well, it's not really specific to this, but it's needed for this test case because it requires parsing more than one input file at once, which the current automated setup doesn't allow. So yeah, it can be used by more test cases in theory, but in practice until now we didn't have a use case.
but it's needed for this test case because it requires parsing more than one input file at once
Thats fine then.
Merged #1991 into master.
github-comments@lists.geany.org