Cleanup Swift Metadata #3533

InsertCreativityHere · 2025-02-11T15:46:36Z

This PR removes the swift:inherits metadata, and implements #3529 (we no longer require re-openings of the same module with swift:module metadatas to have the same 'prefix' argument)

EDIT: It also now changes all the identifiers in the scope tests.

bernardnormier · 2025-02-12T13:38:48Z

cpp/src/slice2swift/SwiftUtil.cpp

@@ -683,36 +670,6 @@ SwiftGenerator::validateSwiftModuleMappings(const UnitPtr& unit)
               << current->second << "'" << endl;


I would keep the message on a single line, no \n.

I didn't even notice this was split up. Fixed.

bernardnormier · 2025-02-12T13:43:37Z

cpp/src/slice2swift/SwiftUtil.cpp

-    ModuleMap mappedModules;
-    // With a given Swift module a Slice module has to map to a single prefix
-    std::map<std::string, ModuleMap> mappedModulePrefixes;
+    // Each Slice unit has to map all top-level modules to a single Swift module.


It would be helpful to describe what this map is. It seems to be a map filename -> swift module name.

The map was deleted.
The previous check was being applied to all files, even included ones.
This seemed over-zealous to me. Now we only check the single file that is being compiler.

And since we're only checking one file, we don't need a file-name map anymore.

bernardnormier · 2025-02-12T13:47:20Z

cpp/src/slice2swift/SwiftUtil.cpp

-    // With a given Swift module a Slice module has to map to a single prefix
-    std::map<std::string, ModuleMap> mappedModulePrefixes;
+    // Each Slice unit has to map all top-level modules to a single Swift module.
+    map<string, string> mappedModules;

    // Any modules that are directly contained on the unit are (by definition) top-level modules.
    // And since there is only one unit per compilation, this must be all the top-level modules.
    for (const auto& module : unit->modules())


I would not use module as a variable name. It's an identifier with special meaning in C++20: https://en.cppreference.com/w/cpp/keyword

Fixed.
I renamed it to mod.

bernardnormier · 2025-02-12T13:48:39Z

swift/test/Ice/scope/AllTests.swift

@@ -201,6 +201,36 @@ func allTests(helper: TestHelper) async throws {
        try test(cmap3["a"]!!.s == s1)
    }

+    do {
+        let obj = try communicator.stringToProxy("dpi:\(helper.getTestEndpoint())")!


No stringToProxy followed by checkedCast in new code, please!

If you want to test ice_isA, call ice_isA.

I was just copy-pasting from the lines above.
I removed all the checkedCast calls from this test; all of them were superfluous.

If you don't remember the preferred syntax, check the top-level CHANGELOG-3.8.md or the greeter demo:

// Swift let greeter = try makeProxy( communicator: communicator, proxyString: "greeter:tcp -h localhost -p 4061", type: GreeterPrx.self)

I wasn't familiar enough with Swift to remember that we supported this there.
I updated scope in Swift to use makeProxy. The other languages were all already correct.

bernardnormier · 2025-02-12T13:53:44Z

swift/test/Ice/scope/Test.ice


-module Inner2
+// Check that it's okay for 'swift:module' to have different 'prefix' arguments.


I am sure we can test this "feature" with a simpler test.

Well, I also want to make sure that it gets mapped to the correct place as well.
Not just that "the compiler doesn't issue an error anymore".

Because we don't have a test for the swift:module anywhere, and scope felt like an adequate place to put it.
Since the point of the metadata is changing the mapped scopes.

bernardnormier · 2025-02-12T13:55:04Z

swift/test/Ice/scope/Test.ice


-        void shutdown();
+    interface I


We should definitely never name an interface 'I'.

I was just being consistent with the rest of this test.
I updated the scope tests to use better names.

bernardnormier · 2025-02-12T13:55:33Z

swift/test/Ice/scope/Test.ice

-        Test::C opC(Test::C c1, out Test::C c2);
-        Test::CSeq opCSeq(Test::CSeq c1, out Test::CSeq c2);
-        Test::CMap opCMap(Test::CMap c1, out Test::CMap c2);
+        long l;


Trying hard to find the worse names?

Nope, just following our old 'conventions'!
I updated almost all of the names in this test to be less horrible.

bernardnormier · 2025-02-17T21:22:56Z

swift/test/Ice/scope/AllTests.swift

@@ -201,6 +201,36 @@ func allTests(helper: TestHelper) async throws {
        try test(cmap3["a"]!!.s == s1)
    }

+    do {
+        let obj = try communicator.stringToProxy("dpi:\(helper.getTestEndpoint())")!


If you don't remember the preferred syntax, check the top-level CHANGELOG-3.8.md or the greeter demo:

// Swift let greeter = try makeProxy( communicator: communicator, proxyString: "greeter:tcp -h localhost -p 4061", type: GreeterPrx.self)

InsertCreativityHere added 6 commits February 11, 2025 10:43

Cleanup Swift metadata.

89cb3bd

Merge branch 'main' into cleanup-swift-metadata

4588954

Accidentally commited generated code.

bca5243

Forgot to hit save.

5229dfe

I misunderstood how 'swift:module' worked again...

2f2afc2

Apparently classes are always optional .

487b57a

InsertCreativityHere requested review from bernardnormier and externl and removed request for bernardnormier February 11, 2025 19:45

InsertCreativityHere marked this pull request as ready for review February 11, 2025 20:58

bernardnormier reviewed Feb 12, 2025

View reviewed changes

InsertCreativityHere added 10 commits February 12, 2025 15:51

Further simplify module-consistency check.

17921dc

Removed 'S2' and fixed uncheckedCast calls.

d49bce0

Removed other 'checkedCast's from the other 'scope' tests.

7015dd0

Some more formatting.

7be510e

Typo.

9e9dbb6

Fixed 'S' and 'C' in the scoped tests.

b04e5f4

Finished updating names in the 'scope' tests.

9c89a45

Formatting.

2c5e4dd

Fixed typo in the ruby tests.

90a7267

Another 'scope' test typo.

711193e

InsertCreativityHere requested a review from bernardnormier February 17, 2025 20:33

Swift test cleanup.

2fbceca

bernardnormier approved these changes Feb 17, 2025

View reviewed changes

InsertCreativityHere added 2 commits February 17, 2025 17:12

Use 'makeProxy' instead of 'uncheckedCast' in Swift.

385f511

Swift formatting.

dad37a6

externl approved these changes Feb 18, 2025

View reviewed changes

InsertCreativityHere merged commit 6dbee6f into zeroc-ice:main Feb 18, 2025
22 of 23 checks passed

InsertCreativityHere mentioned this pull request Feb 18, 2025

Loosen the 'Prefix' Check for swift:module Metadata #3529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Swift Metadata #3533

Cleanup Swift Metadata #3533

InsertCreativityHere commented Feb 11, 2025 •

edited

Loading

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 17, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 12, 2025

InsertCreativityHere Feb 17, 2025

bernardnormier Feb 17, 2025

		@@ -683,36 +670,6 @@ SwiftGenerator::validateSwiftModuleMappings(const UnitPtr& unit)
		<< current->second << "'" << endl;


		module Inner2
		// Check that it's okay for 'swift:module' to have different 'prefix' arguments.

Cleanup Swift Metadata #3533

Cleanup Swift Metadata #3533

Conversation

InsertCreativityHere commented Feb 11, 2025 • edited Loading

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

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

InsertCreativityHere commented Feb 11, 2025 •

edited

Loading