-
Notifications
You must be signed in to change notification settings - Fork 592
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.
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
New makeProxy factory function in Swift #2275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -13,13 +13,10 @@ func allTests(_ helper: TestHelper, collocated: Bool = false) throws { | |||
let output = helper.getWriter() | |||
|
|||
var sref = "test:\(helper.getTestEndpoint(num: 0))" | |||
var obj = try communicator.stringToProxy(sref)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need this intermediary ObjectPrx.
@@ -89,7 +86,7 @@ func allTests(_ helper: TestHelper, collocated: Bool = false) throws { | |||
|
|||
output.write("testing exception callback... ") | |||
do { | |||
let i = uncheckedCast(prx: p.ice_adapterId("dummy"), type: TestIntfPrx.self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The various ice_ factory method already returned a proxy with the same type.
@@ -48,12 +49,8 @@ func allTests(_ helper: TestHelper) throws { | |||
|
|||
try com.deactivateObjectAdapter(adapter) | |||
|
|||
let test3 = uncheckedCast(prx: test1, type: TestIntfPrx.self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code made no sense. uncheckedCast here just returns test1.
@@ -438,9 +431,8 @@ func allTests(_ helper: TestHelper) throws { | |||
|
|||
try com.deactivateObjectAdapter(adapter) | |||
|
|||
let test3 = uncheckedCast(prx: test1, type: TestIntfPrx.self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns test1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I rerun Windows build but still failing. It works locally. Anyway is clearly unrelated to the PR.
swift/test/Ice/ami/AllTests.swift
Outdated
@@ -13,13 +13,10 @@ func allTests(_ helper: TestHelper, collocated: Bool = false) throws { | |||
let output = helper.getWriter() | |||
|
|||
var sref = "test:\(helper.getTestEndpoint(num: 0))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also remove this sref variables, and write the string directly as the "proxyString:" parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in this test.
|
||
let locator = uncheckedCast(prx: communicator.getDefaultLocator()!, type: TestLocatorPrx.self) | ||
|
||
let registry = try checkedCast(prx: locator.getRegistry()!, type: TestLocatorRegistryPrx.self)! | ||
let registry = try uncheckedCast(prx: locator.getRegistry()!, type: TestLocatorRegistryPrx.self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C++ we got rid of uncheckedCast
usage with the constructor, will we do the same with makeProxy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't plan to provide a makeProxy
overload that is an alias for uncheckedCast
.
The situation in C++ is different since we provide a constructor, not another function.
This PR adds a new
makeProxy
factory function to Ice for Swift.It's similar to the existing uncheckedCast and should be preferred over uncheckedCast (and checkedCast) in many situations, in particular in the demos.
It also updated 3 tests to use makeProxy.