Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

InsertCreativityHere · 2025-01-07T16:46:39Z

This PR fixes #3313 and #3205.

All of the instabilities were caused by places where we were calling sort on lists of shared_ptrs, which meant we were sorting things by unstable memory addresses.
Thankfully, all of the sorting we were trying to do in those places was unnecessary and removed.

The two types of sorting which is removed by this PR:

We used to sort the exception specifications of operation, so it'd be safe to call unique to remove duplicates.
But this is useless. The Slice compiler already disallows the same exception to appear twice in an exception specification.
We used to sort out operations by name, so it'd be safe to call unique to remove duplicates.
But again, the Slice compiler issues errors if there's any conflicts, even from inherited names (including diamonds).

One exception:
We still sort our operations by name in C++ because we use a binary search over operation names to perform dispatch.
So we require the names to be alphabetized. We re-use the same list for generating the actual operations and the dispatch, and I just left it alone. It's fine.

…ed code.

InsertCreativityHere · 2025-01-07T16:47:32Z

cpp/src/Slice/Parser.cpp

@@ -3053,7 +3053,6 @@ Slice::InterfaceDef::allBases() const
 {
    InterfaceList result = _bases;
    result.sort(containedCompare);
-    result.unique(containedEqual);


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.

InsertCreativityHere · 2025-01-07T16:47:38Z

cpp/src/Slice/Parser.cpp

@@ -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();


Dead variable.

InsertCreativityHere · 2025-01-07T16:48:11Z

cpp/src/ice2slice/Gen.cpp

-        throws.sort();
-        throws.unique();


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

InsertCreativityHere · 2025-01-07T16:49:47Z

cpp/src/slice2cpp/Gen.cpp

            // 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());


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.

InsertCreativityHere · 2025-01-07T16:50:40Z

cpp/src/slice2cs/Gen.cpp

-            string ns = getNamespace(p);
-            string name = getDispatchParams(*i, retS, params, args, ns);
-            _out << sp << nl << "public abstract " << retS << " " << name << spar << params << epar << ';';
-        }


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

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

InsertCreativityHere · 2025-01-07T16:50:46Z

cpp/src/slice2cs/Gen.cpp

-        // being marshaled instead of the derived exception.
-        //
-        throws.sort(Slice::DerivedToBaseCompare());
-


InsertCreativityHere · 2025-01-07T16:51:18Z

cpp/src/slice2java/Gen.cpp

@@ -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();


Another dead variable...

InsertCreativityHere · 2025-01-07T16:51:41Z

cpp/src/slice2js/Gen.cpp

@@ -2035,7 +2036,6 @@ Slice::Gen::TypeScriptImportVisitor::visitInterfaceDefStart(const InterfaceDefPt
    }

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


InsertCreativityHere · 2025-01-07T16:51:50Z

cpp/src/slice2matlab/Main.cpp

@@ -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();


bernardnormier

I woud also remove:

allOpNames.unique();

from line 2564 in slice2cpp/Gen.cpp.

bernardnormier · 2025-01-07T20:41:10Z

cpp/src/slice2cs/Gen.cpp

@@ -317,26 +317,31 @@ Slice::CsVisitor::writeUnmarshalDataMember(
 void
 Slice::CsVisitor::writeInheritedOperations(const InterfaceDefPtr& p)
 {


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

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.

InsertCreativityHere · 2025-01-07T21:29:47Z

Originally I left that unique in because I was planning to check out the code around it, because I smelled a bug.
But I went ahead and removed this unique in this PR and just opened an issue for my suspicion:
#3318

InsertCreativityHere · 2025-01-07T21:31:28Z

cpp/src/slice2cpp/Gen.cpp

@@ -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();


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

InsertCreativityHere · 2025-01-07T21:32:16Z

cpp/src/slice2cs/Gen.cpp

+            }
+        }
+    }
+    for (const auto& op : allOps)


Where writeInheritedOperations was inlined to.

…ere.

InsertCreativityHere · 2025-01-07T22:44:00Z

cpp/src/slice2cs/Gen.cpp

@@ -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())


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.

InsertCreativityHere added 3 commits January 6, 2025 17:29

Removing sorting/unique-ifying of exception specifications in generat…

bcf95eb

…ed code.

Removed sorting of operation name lists (except in C++).

9ade3bd

Another dead variable...

8f0a440

InsertCreativityHere commented Jan 7, 2025

View reviewed changes

InsertCreativityHere requested review from bernardnormier and pepone January 7, 2025 16:52

InsertCreativityHere added 2 commits January 7, 2025 14:40

Fixes.

91faa63

Clang-format...

156b3fe

bernardnormier approved these changes Jan 7, 2025

View reviewed changes

Fixed review comments.

9af91c3

InsertCreativityHere commented Jan 7, 2025

View reviewed changes

After looking at it more, Bernard is right, 'allOperations' is safe h…

cbc0efc

…ere.

InsertCreativityHere commented Jan 7, 2025

View reviewed changes

pepone approved these changes Jan 8, 2025

View reviewed changes

InsertCreativityHere merged commit 8fcf209 into zeroc-ice:main Jan 8, 2025
21 checks passed

InsertCreativityHere mentioned this pull request Jan 8, 2025

slice2cpp Does Not Generate catch Blocks in a Stable Ordering #3205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

InsertCreativityHere commented Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

bernardnormier left a comment

bernardnormier Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere commented Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

InsertCreativityHere Jan 7, 2025

Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

Fix Instabilities and Unnecessary Sorting in the Generated Code #3315

Conversation

InsertCreativityHere commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InsertCreativityHere commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment