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 5 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
2 changes: 0 additions & 2 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 @@ -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
8 changes: 1 addition & 7 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
65 changes: 24 additions & 41 deletions cpp/src/slice2cs/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,26 +317,31 @@ Slice::CsVisitor::writeUnmarshalDataMember(
void
Slice::CsVisitor::writeInheritedOperations(const InterfaceDefPtr& p)
{
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to eliminate this helper function (called once) and just call allOperations on the interface in the calling function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to make it too hard to review the changes, but you're right.
I 'inlined' this code, and actually managed to simplify a good chunk of the code around where we called this too.

InterfaceList bases = p->bases();
if (!bases.empty())
OperationList allBaseOps;
for (const auto& base : p->bases())
{
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)
for (const auto& baseOp : base->allOperations())
{
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 << ';';
// It's possible to get the same operation name through diamond inheritance.
// But we only want one 'copy' of each operation in our list, to avoid generating duplicate methods.
if (find_if(
allBaseOps.begin(),
allBaseOps.end(),
[name = baseOp->name()](const auto& other) { return other->name() == name; }) == allBaseOps.end())
{
allBaseOps.push_back(baseOp);
}
}
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.

}

for (const auto& op : allBaseOps)
{
string retS;
vector<string> params, args;
string ns = getNamespace(p);
string name = getDispatchParams(op, retS, params, args, ns);
_out << sp << nl << "public abstract " << retS << " " << name << spar << params << epar << ';';
}
}

void
Expand Down Expand Up @@ -2698,18 +2703,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 +2782,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 +2896,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 @@ -3257,7 +3241,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
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