Refactor ConnectionInfo in C# #3187
Conversation
| public string connectionId = ""; | ||
| public readonly string connectionId; | ||
|
|
||
| protected ConnectionInfo(ConnectionInfo underlying) |
There was a problem hiding this comment.
Just like for EndpointInfo: when underlying is not null, we use it to set incoming/adapterName/connectionId.
Note that logically:
- incoming == false => connectionId may be set and adapterName must be empty
- incoming == true => connectionId is empty and adapterName should be (must be?) set
However, we don't enforce it in this code.
| { | ||
| _info = _transceiver.getInfo(); | ||
| } | ||
| catch (LocalException) |
There was a problem hiding this comment.
I remove this catch LocalException and create a "default" ConnectionInfo. I don't understand the logic and it's not exercised by the tests.
There was a problem hiding this comment.
This was introduced here c4035e1
Unfortunately there isn't a linked issue to explain when it throws.
| info.remotePort = Network.endpointPort(_peerAddr); | ||
| } | ||
|
|
||
| if (_state == StateNotConnected) // TODO: eliminate all these states |
There was a problem hiding this comment.
See #3186. Once fixed, we'll keep only the first branch.
| connectionId = underlying.connectionId; | ||
| } | ||
|
|
||
| protected ConnectionInfo(bool incoming, string adapterName, string connectionId) |
There was a problem hiding this comment.
Conversely, when underlying is null, we must provide incoming/adapterName/connectionId to the constructor.
csharp/src/Ice/Connection.cs
Outdated
| } | ||
|
|
||
| internal UDPConnectionInfo(bool incoming, string adapterName, string connectionId) | ||
| : this(incoming, adapterName, connectionId, "", -1, "", -1, "", -1, 0, 0) |
There was a problem hiding this comment.
I would use labels for the "" -1, ... parameters
| { | ||
| _info = _transceiver.getInfo(); | ||
| } | ||
| catch (LocalException) |
There was a problem hiding this comment.
This was introduced here c4035e1
Unfortunately there isn't a linked issue to explain when it throws.
This is a companion PR for #3185 - it simplifies ConnectionInfo by switching to readonly fields + protected or internal constructors.