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

Code tree search structs refactor + lazily expanded flat terms #543

Merged
merged 4 commits into from Apr 12, 2024

Conversation

mezpusz
Copy link
Contributor

@mezpusz mezpusz commented Apr 10, 2024

Two main changes in this pull request:

  • CodeTree::SearchStruct is refactored avoiding duplication of subclasses. Raw array containing value-target pairs is replaced with STL containers for better and easier handling.
  • Added new tag FUN_UNEXPANDED to FlatTerm in which case only the tag, term pointer and number of subentries is filled out first. A call on FlatTerm::Entry::expand fills out the same entries for the arguments. The point is that we expand tha flat term lazily which in some cases would be a bottleneck when the FlatTerm is not even used.

Additionally, added some basic printing for code trees.

Copy link
Contributor

@MichaelRawson MichaelRawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me, see comments. As usual I trust you with the details of what you're actually doing but it looks generally sensible and there's less hold-on-to-your-hat pointer manouevring going on, so I'm happy about that!

Kernel/FlatTerm.hpp Outdated Show resolved Hide resolved
Indexing/ClauseCodeTree.cpp Show resolved Hide resolved
Kernel/FlatTerm.cpp Outdated Show resolved Hide resolved
Kernel/FlatTerm.cpp Outdated Show resolved Hide resolved
Indexing/CodeTree.hpp Show resolved Hide resolved
Indexing/CodeTree.hpp Show resolved Hide resolved
@@ -35,6 +35,9 @@
namespace Indexing
{

#define GET_CONTAINING_OBJECT(ContainingClass,MemberField,object) \
reinterpret_cast<ContainingClass*>(reinterpret_cast<size_t>(object)-offsetof(ContainingClass,MemberField))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks dubious to me as I don't think size_t is guaranteed to be big enough. What I think you want to do it cast to char * and then do the arithmetic on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this is also implemented on Linux. The main problem is with char* we need to const_cast as well, because it is called with both const and non-const arguments. Let me know which one do you think is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two macros, one for each case? Could also then be static_cast, I think.

Indexing/CodeTree.cpp Show resolved Hide resolved
Indexing/CodeTree.cpp Show resolved Hide resolved
Indexing/CodeTree.cpp Show resolved Hide resolved
Kernel/FlatTerm.hpp Outdated Show resolved Hide resolved
Lib/BitUtils.hpp Outdated Show resolved Hide resolved
@MichaelRawson
Copy link
Contributor

Thanks for the fixes/improvements, @mezpusz! I'm happy apart from the GET_CONTAINING_OBJECT macro when you want to take a look, @quickbeam123.

@quickbeam123
Copy link
Collaborator

No new assertion violations and slightly improves performance. I think it's ready to go!

@MichaelRawson
Copy link
Contributor

OK! I'll wait to see how/if @mezpusz wants to fix GET_CONTAINING_OBJECT and then we can merge, I think.

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.

None yet

3 participants