Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracon-set.c grow_table(): Use MAX_TRACON_SET_TABLE_SIZE #1517

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

ampli
Copy link
Member

@ampli ampli commented Apr 27, 2024

This fixes a bug in tracon-set.c, existing there since I created it.
The use of MAX_STRING_SET_TABLE_SIZE in grow_table() causes an inconsistent setting of the maximum loading factor, as tracon_set_create() and tracon_set_reset() use MAX_TRACON_SET_TABLE_SIZE and these definitions are now not the same.

After this fix, the maximum load factor of the tracon-set is always 3/8, as specified in tracon-set.h (in string-set.h and string-id.h it got set to 3/4 in commit 397a3d4). The run time doesn't noticeably improve, which may hint at a possible increase of MAX_TRACON_SET_TABLE_SIZE.

The unintentional use of MAX_STRING_SET_TABLE_SIZE went unnoticed because string-set.h is pulled into tracon-set.c through the inclusion of connectors.h. It would be better if internal module definitions were kept private and did not appear in internal API include files. So I propose to move the private string-set.c definitions from string-set.h to string-set.c, and the like for the other modules.

(This line is for a backreference in issue #1487, in which I mentioned this PR.)

This bug introduced in the initial version of this file.
@linas linas merged commit 0f9ae60 into opencog:master Apr 27, 2024
1 check passed
@linas
Copy link
Member

linas commented Apr 27, 2024

So I propose to move the private string-set.c definitions from string-set.h to string-set.c

Yes, this seems like a good idea. I was surprised to see the similar names in the header file, and kept thinking "this is dangerous and will lead to bugs" but I didn't actually try to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants