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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Cleanup 'addImport' and Surrounding Functions in slice2swift #3371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 29 additions & 78 deletions cpp/src/slice2swift/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,18 +132,10 @@ Gen::ImportVisitor::visitModuleStart(const ModulePtr& p)
bool
Gen::ImportVisitor::visitClassDefStart(const ClassDefPtr& p)
{
//
// Add imports required for base class
//
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.

}
addImport(p->base(), p);

//
// Add imports required for data members
//
const DataMemberList allDataMembers = p->allDataMembers();
for (const auto& allDataMember : allDataMembers)
{
Expand All @@ -156,29 +148,19 @@ Gen::ImportVisitor::visitClassDefStart(const ClassDefPtr& p)
bool
Gen::ImportVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
//
// Add imports required for base interfaces
//
InterfaceList bases = p->bases();
for (const auto& base : bases)
{
addImport(dynamic_pointer_cast<Contained>(base), p);
addImport(base, p);
}

//
// Add imports required for operation parameters and return type
//
const OperationList operationList = p->allOperations();
for (const auto& i : operationList)
for (const auto& operation : p->allOperations())
{
const TypePtr ret = i->returnType();
if (ret && ret->definitionContext())
{
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.


const ParameterList paramList = i->parameters();
for (const auto& j : paramList)
for (const auto& j : operation->parameters())
{
addImport(j->type(), p);
}
Expand All @@ -190,11 +172,8 @@ Gen::ImportVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
bool
Gen::ImportVisitor::visitStructStart(const StructPtr& p)
{
//
// Add imports required for data members
//
const DataMemberList dataMembers = p->dataMembers();
for (const auto& dataMember : dataMembers)
for (const auto& dataMember : p->dataMembers())
{
addImport(dataMember->type(), p);
}
Expand All @@ -205,41 +184,28 @@ Gen::ImportVisitor::visitStructStart(const StructPtr& p)
bool
Gen::ImportVisitor::visitExceptionStart(const ExceptionPtr& p)
{
//
// Add imports required for base exceptions
//
ExceptionPtr base = p->base();
if (base)
{
addImport(dynamic_pointer_cast<Contained>(base), p);
}
addImport(p->base(), p);

//
// Add imports required for data members
//
const DataMemberList allDataMembers = p->allDataMembers();
for (const auto& allDataMember : allDataMembers)
for (const auto& member : p->allDataMembers())
{
addImport(allDataMember->type(), p);
addImport(member->type(), p);
}
return true;
}

void
Gen::ImportVisitor::visitSequence(const SequencePtr& seq)
{
//
// Add import required for the sequence element type
//
addImport(seq->type(), seq);
}

void
Gen::ImportVisitor::visitDictionary(const DictionaryPtr& dict)
{
//
// Add imports required for the dictionary key and value types
//
addImport(dict->keyType(), dict);
addImport(dict->valueType(), dict);
}
Expand All @@ -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.

Gen::ImportVisitor::addImport(const SyntaxTreeBasePtr& p, const ContainedPtr& toplevel)
{
if (!dynamic_pointer_cast<Builtin>(definition))
// Only add imports for user defined constructs.
auto definition = dynamic_pointer_cast<Contained>(p);
if (definition)
{
ModulePtr m1 = getTopLevelModule(definition);
ModulePtr m2 = getTopLevelModule(toplevel);

string swiftM1 = getSwiftModule(m1);
string swiftM2 = getSwiftModule(m2);
if (swiftM1 != swiftM2 && find(_imports.begin(), _imports.end(), swiftM1) == _imports.end())
string swiftM1 = getSwiftModule(getTopLevelModule(definition));
string swiftM2 = getSwiftModule(getTopLevelModule(toplevel));
if (swiftM1 != swiftM2)
{
_imports.push_back(swiftM1);
addImport(swiftM1);
}
}
}

void
Gen::ImportVisitor::addImport(const ContainedPtr& definition, const ContainedPtr& toplevel)
{
ModulePtr m1 = getTopLevelModule(definition);
ModulePtr m2 = getTopLevelModule(toplevel);

string swiftM1 = getSwiftModule(m1);
string swiftM2 = getSwiftModule(m2);
if (swiftM1 != swiftM2 && find(_imports.begin(), _imports.end(), swiftM1) == _imports.end())
{
_imports.push_back(swiftM1);
}
}

void
Gen::ImportVisitor::addImport(const string& module)
{
Expand All @@ -298,7 +249,7 @@ Gen::TypesVisitor::TypesVisitor(IceInternal::Output& o) : out(o) {}
bool
Gen::TypesVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = fixIdent(getRelativeTypeString(p, swiftModule));
const string traits = fixIdent(getRelativeTypeString(p, swiftModule) + "Traits");

Expand Down Expand Up @@ -330,7 +281,7 @@ Gen::TypesVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
bool
Gen::TypesVisitor::visitExceptionStart(const ExceptionPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = getRelativeTypeString(p, swiftModule);

ExceptionPtr base = p->base();
Expand Down Expand Up @@ -474,7 +425,7 @@ Gen::TypesVisitor::visitExceptionStart(const ExceptionPtr& p)
bool
Gen::TypesVisitor::visitStructStart(const StructPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = fixIdent(getRelativeTypeString(p, swiftModule));
bool isLegalKeyType = Dictionary::isLegalKeyType(p);
const DataMemberList members = p->dataMembers();
Expand Down Expand Up @@ -590,7 +541,7 @@ Gen::TypesVisitor::visitStructStart(const StructPtr& p)
void
Gen::TypesVisitor::visitSequence(const SequencePtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = getRelativeTypeString(p, swiftModule);

const TypePtr type = p->type();
Expand Down Expand Up @@ -745,7 +696,7 @@ Gen::TypesVisitor::visitSequence(const SequencePtr& p)
void
Gen::TypesVisitor::visitDictionary(const DictionaryPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = getRelativeTypeString(p, swiftModule);

const string keyType = typeToString(p->keyType(), p, false);
Expand Down Expand Up @@ -894,7 +845,7 @@ Gen::TypesVisitor::visitDictionary(const DictionaryPtr& p)
void
Gen::TypesVisitor::visitEnum(const EnumPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = fixIdent(getRelativeTypeString(p, swiftModule));
const EnumeratorList enumerators = p->enumerators();
const string enumType = p->maxValue() <= 0xFF ? "Swift.UInt8" : "Swift.Int32";
Expand Down Expand Up @@ -991,7 +942,7 @@ Gen::TypesVisitor::visitConst(const ConstPtr& p)
{
const string name = fixIdent(p->name());
const TypePtr type = p->type();
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));

writeDocSummary(out, p);
out << nl << "public let " << name << ": " << typeToString(type, p) << " = ";
Expand All @@ -1012,7 +963,7 @@ Gen::ProxyVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
InterfaceList bases = p->bases();

const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = getRelativeTypeString(p, swiftModule);
const string traits = name + "Traits";
const string prx = name + "Prx";
Expand Down Expand Up @@ -1197,7 +1148,7 @@ bool
Gen::ValueVisitor::visitClassDefStart(const ClassDefPtr& p)
{
const string prefix = getClassResolverPrefix(p->unit());
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = getRelativeTypeString(p, swiftModule);

ClassDefPtr base = p->base();
Expand Down Expand Up @@ -1350,7 +1301,7 @@ Gen::ObjectVisitor::ObjectVisitor(::IceInternal::Output& o) : out(o) {}
bool
Gen::ObjectVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string disp = fixIdent(getRelativeTypeString(p, swiftModule) + "Disp");
const string traits = fixIdent(getRelativeTypeString(p, swiftModule) + "Traits");
const string servant = fixIdent(getRelativeTypeString(p, swiftModule));
Expand Down Expand Up @@ -1471,7 +1422,7 @@ Gen::ObjectVisitor::visitInterfaceDefEnd(const InterfaceDefPtr&)
void
Gen::ObjectVisitor::visitOperation(const OperationPtr& op)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(op)));
const string swiftModule = getSwiftModule(getTopLevelModule(op));
const string opName = fixIdent(op->name());
const ParamInfoList allInParams = getAllInParams(op);
const ParamInfoList allOutParams = getAllOutParams(op);
Expand Down Expand Up @@ -1501,7 +1452,7 @@ Gen::ObjectExtVisitor::ObjectExtVisitor(::IceInternal::Output& o) : out(o) {}
bool
Gen::ObjectExtVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(p)));
const string swiftModule = getSwiftModule(getTopLevelModule(p));
const string name = getRelativeTypeString(p, swiftModule);

out << sp;
Expand Down
5 changes: 2 additions & 3 deletions cpp/src/slice2swift/Gen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,8 @@ namespace Slice
void writeImports();

private:
void addImport(const TypePtr&, const ContainedPtr&);
void addImport(const ContainedPtr&, const ContainedPtr&);
void addImport(const std::string&);
void addImport(const SyntaxTreeBasePtr& p, const ContainedPtr& toplevel);
void addImport(const std::string& module);

IceInternal::Output& out;
std::vector<std::string> _imports;
Expand Down
22 changes: 5 additions & 17 deletions cpp/src/slice2swift/SwiftUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,7 @@ Slice::getSwiftModule(const ModulePtr& module)
ModulePtr
Slice::getTopLevelModule(const ContainedPtr& cont)
{
//
// Traverse to the top-level module.
//
ModulePtr m;
ContainedPtr p = cont;
while (true)
Expand All @@ -257,16 +255,6 @@ Slice::getTopLevelModule(const ContainedPtr& cont)
return m;
}

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.

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));
}

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.

assert(dynamic_pointer_cast<InterfaceDecl>(type) || dynamic_pointer_cast<Contained>(type));

InterfaceDeclPtr proxy = dynamic_pointer_cast<InterfaceDecl>(type);
return getTopLevelModule(proxy ? dynamic_pointer_cast<Contained>(proxy) : dynamic_pointer_cast<Contained>(type));
}

void
SwiftGenerator::writeDocLines(IceInternal::Output& out, const StringList& lines, bool commentFirst, const string& space)
{
Expand Down Expand Up @@ -993,7 +981,7 @@ SwiftGenerator::writeMembers(
(dynamic_pointer_cast<Struct>(type) || dynamic_pointer_cast<Sequence>(type) ||
dynamic_pointer_cast<Dictionary>(type)))
{
ModulePtr m = getTopLevelModule(type);
ModulePtr m = getTopLevelModule(dynamic_pointer_cast<Contained>(type));
alias = m->name() + "_" + memberType;
out << nl << "typealias " << alias << " = " << memberType;
}
Expand Down Expand Up @@ -1160,7 +1148,7 @@ SwiftGenerator::writeMarshalUnmarshalCode(
//
if (memberType == memberName)
{
ModulePtr m = getTopLevelModule(type);
ModulePtr m = getTopLevelModule(cl);
alias = m->name() + "_" + memberType;
out << nl << "typealias " << alias << " = " << memberType;
}
Expand Down Expand Up @@ -1832,7 +1820,7 @@ SwiftGenerator::writeUnmarshalInParams(::IceInternal::Output& out, const Operati
void
SwiftGenerator::writeUnmarshalUserException(::IceInternal::Output& out, const OperationPtr& op)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(op)));
const string swiftModule = getSwiftModule(getTopLevelModule(op));

// Arrange exceptions into most-derived to least-derived order. If we don't
// do this, a base exception handler can appear before a derived exception
Expand Down Expand Up @@ -1880,7 +1868,7 @@ SwiftGenerator::writeProxyOperation(::IceInternal::Output& out, const OperationP
const ParamInfoList allOutParams = getAllOutParams(op);
const ExceptionList allExceptions = op->throws();

const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(op)));
const string swiftModule = getSwiftModule(getTopLevelModule(op));

out << sp;
writeOpDocSummary(out, op, false);
Expand Down Expand Up @@ -1962,7 +1950,7 @@ SwiftGenerator::writeDispatchOperation(::IceInternal::Output& out, const Operati
const ParamInfoList inParams = getAllInParams(op);
const ParamInfoList outParams = getAllOutParams(op);

const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(op)));
const string swiftModule = getSwiftModule(getTopLevelModule(op));

out << sp;
out << nl << "public func _iceD_" << opName
Expand Down
1 change: 0 additions & 1 deletion cpp/src/slice2swift/SwiftUtil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ namespace Slice
std::string getSwiftModule(const ModulePtr&, std::string&);
std::string getSwiftModule(const ModulePtr&);
ModulePtr getTopLevelModule(const ContainedPtr&);
ModulePtr getTopLevelModule(const TypePtr&);

std::string fixIdent(const std::string&);

Expand Down
Loading