-
Notifications
You must be signed in to change notification settings - Fork 593
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
Cleanup 'addImport' and Surrounding Functions in slice2swift
#3371
Conversation
ClassDefPtr base = p->base(); | ||
if (base) | ||
{ | ||
addImport(dynamic_pointer_cast<Contained>(base), p); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
ice/cpp/src/slice2swift/SwiftUtil.cpp
Lines 213 to 221 in d53f0a9
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)); | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
In
slice2swift
we have two versions ofgetTopLevelModule
andaddImport
.One takes a
ContainedPtr
and one takes aTypePtr
.But, the versions which took
TypePtr
were weird and not really necessary.They basically just converted the
TypePtr
into aContainedPtr
to call the other one.But most of the places where it was called, the
Type
was already aContained
.Anyways, I removed the
TypePtr
versions.And because of this, alot of unnecessary
dynamic_pointer_cast
s.