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

Unit Cycle Fixes & Cleanup SyntaxTreeBase #3496

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

InsertCreativityHere
Copy link
Member

The classes we use to represent Slice definitions in the parser rely heavily on inheritance. The base of all these types is SyntaxTreeBase. This PR's focus is on moving stuff out of SyntaxTreeBase and into more appropriate places,
further up the inheritance tree.

Specifically:

  • SyntaxTreeBase has a default implementation of visit which is no-op. This is bad.
    It means that you can call Builtin::visit. But what does it even mean to visit a built in type?
    This visit function was moved up to Contained, which represents a Slice definition.
    These are the only sensible things to visit.
    Also, visit is now pure virtual. Every type should (and does) implement it's own visit logic.

  • SyntaxTreeBase also holds a UnitPtr field. This was removed.
    We had to jump through hoops to get this to work for Unit, since this means it holds a shared_ptr to itself... Also, it made us unable to provide a default constructor for SyntaxTreeBase.
    So. This field was removed and replaced with a pure virtual unit() function.
    Contained holds a UnitPtr field, and returns it in it's implementation.
    Whereas Unit implements this function with shared_from_this.

  • By slimming down SyntaxTreeBase in these ways, I was able to completely eliminate it's constructor, along with the constructors for Type and Container.


A result of this is that #3358 is fixed.

@InsertCreativityHere InsertCreativityHere changed the title Unit cycle fixes Unit Cycle Fixed & Cleanup SyntaxTreeBase Feb 4, 2025
@InsertCreativityHere InsertCreativityHere changed the title Unit Cycle Fixed & Cleanup SyntaxTreeBase Unit Cycle Fixes & Cleanup SyntaxTreeBase Feb 4, 2025
Copy link
Member

@bernardnormier bernardnormier left a comment

Choose a reason for hiding this comment

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

It would be helpeful to explain which cycles the various destroy() functions break.

@@ -356,7 +347,9 @@ namespace Slice
KindValue
};

Builtin(const UnitPtr& unit, Kind kind);
Builtin(UnitPtr unit, Kind kind);
void destroy();
Copy link
Member

@bernardnormier bernardnormier Feb 4, 2025

Choose a reason for hiding this comment

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

I don't understand the logic of destroy(). Which cycle are you breaking?

Update: ok, it's because Unit holds onto a list of Builtins. The modern approach is for the Builtin to hold a weak_ptr<Unit>. This way, no cycle and no need for Builtin::destroy.

_name(std::move(name))
: _container(container),
_name(std::move(name)),
_unit(container->unit())
Copy link
Member

Choose a reason for hiding this comment

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

I am puzzled by this code: instead of keeping _unit, you could just return _container->unit(), and then you don't need any destroy, since destroy() doesn't clear _container.

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