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

Cleanup 'addImport' and Surrounding Functions in slice2swift #3371

Conversation

InsertCreativityHere
Copy link
Member

In slice2swift we have two versions of getTopLevelModule and addImport.
One takes a ContainedPtr and one takes a TypePtr.

But, the versions which took TypePtr were weird and not really necessary.
They basically just converted the TypePtr into a ContainedPtr to call the other one.
But most of the places where it was called, the Type was already a Contained.

Anyways, I removed the TypePtr versions.
And because of this, alot of unnecessary dynamic_pointer_casts.

ClassDefPtr base = p->base();
if (base)
{
addImport(dynamic_pointer_cast<Contained>(base), p);
Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine to pass a nullptr to addImport.
So checking base before calling it is unnecessary.

Also no need to cast to Contained now. ClassDef already inherits from Contained.

@@ -254,36 +220,21 @@ Gen::ImportVisitor::writeImports()
}

void
Gen::ImportVisitor::addImport(const TypePtr& definition, const ContainedPtr& toplevel)
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of having 2 addImport which do the same thing,
now we only have 1 which takes the common base class of Type and Contained: SyntaxTreeBase.

ModulePtr
Slice::getTopLevelModule(const TypePtr& type)
{
// TODO this check is redundant. `InterfaceDecl` _is_ a `Contained`. This whole function is weird though.
Copy link
Member

Choose a reason for hiding this comment

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

The original code in 3.7 made sense.

ModulePtr
Slice::getTopLevelModule(const TypePtr& type)
{
assert(ProxyPtr::dynamicCast(type) || ContainedPtr::dynamicCast(type));
ProxyPtr proxy = ProxyPtr::dynamicCast(type);
return getTopLevelModule(proxy ? ContainedPtr::dynamicCast(proxy->_class()) :
ContainedPtr::dynamicCast(type));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense since back then we didn't have an Interface type in the compiler. Just Class and Proxy.
So, when all this got shuffled around, I guess that this function was mechanistically updated into a useless form.

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

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

Looks good to me!

{
addImport(ret, p);
}
addImport(operation->returnType(), p);
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need to check ret->definitionContext() anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because:
dynamic_pointer_cast<Contained>(p) == true
implies that
p->definitionContext() != null

Both of these are about checking the same thing: "is this a user-defined type?"

Because any user defined check will have a definitionContext (as opposed to something like int, a built-in type).
And will also be a subclass of Contained in the compiler.

@InsertCreativityHere InsertCreativityHere merged commit 7ddc388 into zeroc-ice:main Jan 16, 2025
23 checks passed
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