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

Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

Merged
merged 7 commits into from
Jan 8, 2025
Merged

Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bcf95eb
Removing sorting/unique-ifying of exception specifications in generat…
InsertCreativityHere Jan 6, 2025
9ade3bd
Removed sorting of operation name lists (except in C++).
InsertCreativityHere Jan 7, 2025
8f0a440
Another dead variable...
InsertCreativityHere Jan 7, 2025
91faa63
Fixes.
InsertCreativityHere Jan 7, 2025
156b3fe
Clang-format...
InsertCreativityHere Jan 7, 2025
9af91c3
Fixed review comments.
InsertCreativityHere Jan 7, 2025
cbc0efc
After looking at it more, Bernard is right, 'allOperations' is safe h…
InsertCreativityHere Jan 7, 2025
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
6 changes: 2 additions & 4 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3053,7 +3053,6 @@ Slice::InterfaceDef::allBases() const
{
InterfaceList result = _bases;
result.sort(containedCompare);
result.unique(containedEqual);
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 is illegal to list the same interface multiple times in an inheritance list, so there's no point to calling unique on _bases here.

for (const auto& p : _bases)
{
result.merge(p->allBases(), containedCompare);
Expand Down Expand Up @@ -3088,7 +3087,7 @@ Slice::InterfaceDef::allOperations() const
if (find_if(
result.begin(),
result.end(),
[scoped = q->scoped()](const auto& other) { return other->scoped() == scoped; }) == result.end())
[name = q->name()](const auto& other) { return other->name() == name; }) == result.end())
{
result.push_back(q);
}
Expand All @@ -3100,7 +3099,7 @@ Slice::InterfaceDef::allOperations() const
if (find_if(
result.begin(),
result.end(),
[scoped = q->scoped()](const auto& other) { return other->scoped() == scoped; }) == result.end())
[name = q->name()](const auto& other) { return other->name() == name; }) == result.end())
{
result.push_back(q);
}
Expand Down Expand Up @@ -3584,7 +3583,6 @@ Slice::Exception::createDataMember(
checkIdentifier(name); // Don't return here -- we create the data member anyway.

// Check whether any bases have defined a member with the same name already.
ExceptionList bl = allBases();
Copy link
Member Author

Choose a reason for hiding this comment

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

Dead variable.

for (const auto& q : allBases())
{
ContainedList contents;
Expand Down
2 changes: 0 additions & 2 deletions cpp/src/ice2slice/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,8 +561,6 @@ Gen::TypesVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
}

ExceptionList throws = op->throws();
throws.sort();
throws.unique();
Comment on lines -564 to -565
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer sort our exception specifications, because there's no point to calling unique.
The Slice compiler already ensures this for us.

if (throws.size() == 1)
{
out << " throws " << getUnqualified(throws.front(), scope);
Expand Down
9 changes: 1 addition & 8 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,10 @@ namespace
}
else
{
throws.sort();
throws.unique();

//
// 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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
//
throws.sort(Slice::DerivedToBaseCompare());
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that in the languages, we do still sort them by derived-to-base.
This sorting is stable (it won't re-arrange elements that are 'equal'). And this is necessary to prevent dead catch blocks.

We do this in all compilers except 'ice2slice' (where it's correct not to) and 'slice2php', 'slice2py', and 'slice2rb'.
It is suspicious that we aren't sorting exceptions in those languages.


C << "[](const " << getUnqualified("::Ice::UserException&", scope) << " ex)";
Expand All @@ -233,9 +228,8 @@ namespace
C << sb;
C << nl << "ex.ice_throw();";
C << eb;
//

// Generate a catch block for each legal user exception.
//
for (const auto& ex : throws)
{
C << nl << "catch(const " << getUnqualified(fixKwd(ex->scoped()), scope) << "&)";
Expand Down Expand Up @@ -2657,7 +2651,6 @@ Slice::Gen::InterfaceVisitor::visitInterfaceDefEnd(const InterfaceDefPtr& p)
allOpNames.push_back("ice_isA");
allOpNames.push_back("ice_ping");
allOpNames.sort();
allOpNames.unique();
Copy link
Member Author

Choose a reason for hiding this comment

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

The Slice compiler ensures that there are no duplicate operation names, even among inherited operations.


H << sp;
H << nl << "/// \\cond INTERNAL";
Expand Down
58 changes: 4 additions & 54 deletions cpp/src/slice2cs/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,31 +314,6 @@ Slice::CsVisitor::writeUnmarshalDataMember(
}
}

void
Slice::CsVisitor::writeInheritedOperations(const InterfaceDefPtr& p)
{
InterfaceList bases = p->bases();
if (!bases.empty())
{
OperationList allOps;
for (InterfaceList::const_iterator q = bases.begin(); q != bases.end(); ++q)
{
OperationList tmp = (*q)->allOperations();
allOps.splice(allOps.end(), tmp);
}
allOps.sort();
allOps.unique();
for (OperationList::const_iterator i = allOps.begin(); i != allOps.end(); ++i)
{
string retS;
vector<string> params, args;
string ns = getNamespace(p);
string name = getDispatchParams(*i, retS, params, args, ns);
_out << sp << nl << "public abstract " << retS << " " << name << spar << params << epar << ';';
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Simplified this whole block.
But the main change was removing the sorting of OperationPtr that it used to be doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

After a review comment from Bernard, I just inlined the simplified version of this function.

}
}

void
Slice::CsVisitor::writeMarshaling(const ClassDefPtr& p)
{
Expand Down Expand Up @@ -2698,18 +2673,6 @@ Slice::Gen::HelperVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)

ParameterList outParams = op->outParameters();

ExceptionList throws = op->throws();
throws.sort();
throws.unique();

//
// 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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
//
throws.sort(Slice::DerivedToBaseCompare());

Copy link
Member Author

Choose a reason for hiding this comment

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

Dead code.

string context = getEscapedParamName(op, "context");

_out << sp;
Expand Down Expand Up @@ -2789,21 +2752,14 @@ Slice::Gen::HelperVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)

string returnTypeS = resultType(op, ns);

ExceptionList throws = op->throws();
throws.sort();
throws.unique();

//
// 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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
//
ExceptionList throws = op->throws();
throws.sort(Slice::DerivedToBaseCompare());

//
// Write the public Async method.
//
_out << sp;
_out << nl << "public global::System.Threading.Tasks.Task";
if (!returnTypeS.empty())
Expand Down Expand Up @@ -2910,12 +2866,10 @@ Slice::Gen::HelperVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
_out << nl << "throw ex;";
_out << eb;

//
// Generate a catch block for each legal user exception.
//
for (ExceptionList::const_iterator i = throws.begin(); i != throws.end(); ++i)
for (const auto& thrown : throws)
{
_out << nl << "catch(" << getUnqualified(*i, ns) << ")";
_out << nl << "catch(" << getUnqualified(thrown, ns) << ")";
_out << sb;
_out << nl << "throw;";
_out << eb;
Expand Down Expand Up @@ -3223,17 +3177,14 @@ Slice::Gen::DispatcherVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
_out << fixId(name);

_out << sb;

for (const auto& op : p->operations())
for (const auto& op : p->allOperations())
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we just call allOperations instead of operations followed by writeInheritedOperations.

A side effect of this is that now, the order that we generate operations in is reversed. It used to be Derived -> Base, but now it's Derived -> Base.

{
string retS;
vector<string> params, args;
string opName = getDispatchParams(op, retS, params, args, ns);
_out << sp << nl << "public abstract " << retS << " " << opName << spar << params << epar << ';';
}

writeInheritedOperations(p);

_out << sp;
_out << nl << "public override string ice_id(Ice.Current current) => ice_staticId();";

Expand All @@ -3257,7 +3208,6 @@ Slice::Gen::DispatcherVisitor::writeDispatch(const InterfaceDefPtr& p)
string scoped = p->scoped();
string ns = getNamespace(p);

OperationList ops = p->operations();
OperationList allOps = p->allOperations();
if (!allOps.empty())
{
Expand Down
1 change: 0 additions & 1 deletion cpp/src/slice2cs/Gen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ namespace Slice
void writeMarshalDataMember(const DataMemberPtr&, const std::string&, const std::string&, bool = false);
void writeUnmarshalDataMember(const DataMemberPtr&, const std::string&, const std::string&, bool = false);

void writeInheritedOperations(const InterfaceDefPtr&);
void writeMarshaling(const ClassDefPtr&);

static std::vector<std::string> getParams(const OperationPtr&, const std::string&);
Expand Down
11 changes: 1 addition & 10 deletions cpp/src/slice2java/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1302,8 +1302,6 @@ Slice::JavaVisitor::writeDispatch(Output& out, const InterfaceDefPtr& p)
const bool amd = p->hasMetadata("amd") || op->hasMetadata("amd");

ExceptionList throws = op->throws();
throws.sort();
throws.unique();

out << sp;
writeServantDocComment(out, op, package, dc, amd);
Expand Down Expand Up @@ -3956,8 +3954,6 @@ Slice::Gen::ProxyVisitor::ProxyVisitor(const string& dir) : JavaVisitor(dir) {}
bool
Slice::Gen::ProxyVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
const OperationList ops = p->allOperations();
Copy link
Member Author

Choose a reason for hiding this comment

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

Another dead variable...


string name = p->name();
InterfaceList bases = p->bases();
string package = getPackage(p);
Expand Down Expand Up @@ -4209,16 +4205,11 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
}
const vector<string> args = getInArgs(p);

ExceptionList throws = p->throws();
throws.sort();
throws.unique();

//
// 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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
//
ExceptionList throws = p->throws();
throws.sort(Slice::DerivedToBaseCompare());

const string contextParamName = getEscapedParamName(p, "context");
Expand Down
10 changes: 5 additions & 5 deletions cpp/src/slice2js/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1456,13 +1456,14 @@ Slice::Gen::TypesVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
}
_out << ",";

//
// User exceptions.
//
// 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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
ExceptionList throws = op->throws();
throws.sort();
throws.unique();
throws.sort(Slice::DerivedToBaseCompare());

if (throws.empty())
{
_out << " ";
Expand Down Expand Up @@ -2035,7 +2036,6 @@ Slice::Gen::TypeScriptImportVisitor::visitInterfaceDefStart(const InterfaceDefPt
}

// Add imports required for operation parameters and return types.
const OperationList operationList = p->allOperations();
Copy link
Member Author

Choose a reason for hiding this comment

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

Dead.

for (const auto& op : p->allOperations())
{
auto ret = dynamic_pointer_cast<Contained>(op->returnType());
Expand Down
7 changes: 1 addition & 6 deletions cpp/src/slice2matlab/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,7 +1713,6 @@ CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
const string scoped = p->scoped();
const string abs = getAbsolute(p);
InterfaceList bases = p->bases();
const OperationList allOps = p->allOperations();
Copy link
Member Author

Choose a reason for hiding this comment

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

Dead.


//
// Generate proxy class.
Expand Down Expand Up @@ -2198,16 +2197,12 @@ CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
for (OperationList::const_iterator q = ops.begin(); q != ops.end(); ++q)
{
OperationPtr op = *q;
ExceptionList exceptions = op->throws();
exceptions.sort();
exceptions.unique();

//
// 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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
//
ExceptionList exceptions = op->throws();
exceptions.sort(Slice::DerivedToBaseCompare());

if (!exceptions.empty())
Expand Down
1 change: 1 addition & 0 deletions cpp/src/slice2php/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,7 @@ CodeVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
_out << "null";
}
_out << ", ";

ExceptionList exceptions = (*oli)->throws();
if (!exceptions.empty())
{
Expand Down
3 changes: 0 additions & 3 deletions cpp/src/slice2swift/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,8 +1391,6 @@ Gen::ObjectVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
allOpNames.push_back("ice_ids");
allOpNames.push_back("ice_isA");
allOpNames.push_back("ice_ping");
allOpNames.sort();
allOpNames.unique();

out << sp;
out << nl;
Expand Down Expand Up @@ -1481,7 +1479,6 @@ Gen::ObjectVisitor::visitOperation(const OperationPtr& op)
const string opName = fixIdent(op->name());
const ParamInfoList allInParams = getAllInParams(op);
const ParamInfoList allOutParams = getAllOutParams(op);
const ExceptionList allExceptions = op->throws();

out << sp;
writeOpDocSummary(out, op, true);
Expand Down
8 changes: 6 additions & 2 deletions cpp/src/slice2swift/SwiftUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1820,9 +1820,13 @@ void
SwiftGenerator::writeUnmarshalUserException(::IceInternal::Output& out, const OperationPtr& op)
{
const string swiftModule = getSwiftModule(getTopLevelModule(dynamic_pointer_cast<Contained>(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
// handler, causing compiler warnings and resulting in the base exception
// being marshaled instead of the derived exception.
ExceptionList throws = op->throws();
throws.sort();
throws.unique();
throws.sort(Slice::DerivedToBaseCompare());

out << "{ ex in";
out.inc();
Expand Down
Loading