On Mon, 12 Sep 2011 22:13:47 +0300, Sean Kelly <[email protected]>
wrote:
Looks much nicer than the current std.socket. A few random comments
from a quick scan of the code:
Socket.send/receive should use ubyte[], not void[] for the data.
I'd say this is debatable (e.g. File.rawWrite is templated to the same
effect).
It can't be changed without breaking compatibility, but it could be
possible to add overloads and deprecate the void[] versions.
I'd like some way to avoid new objects being created during any
low-level socket operation I expect to do regularly. For example,
socket.receiveFrom creates a new Address instance every time it's
called. Perhaps I could have the option to supply an Address object to
be overwritten instead?
Good point. Luckily, this particular case has a simple and
backwards-compatible fix:
https://github.com/CyberShadow/phobos/commit/2fbb7d6287ccd760f4e1a6c91acb60f05bf52ed8
That Address.name() returns a sockaddr* is kind of weird. I'd expect it
to return a string? I know that the sockaddr is generally called a
"name" in API parlance, but it seems a bit weird in this context.
Another oddity of the original design. Generally, we're free to rename
methods and schedule aliases for old names for deprecation - and this
method shouldn't have much use outside std.socket anyway. What would be a
better name?
Why is InternetHost an instantiable object? It has data fields that
aren't initialized by any ctor, but only by calls where a hostent* is
passed? And all for access to API calls which no one is supposed to use
anyway? Please just make this go away :-)
I'm not sure what to do about it. It's in use by current code.
The Service and Protocol classes work in a similar manner (fields
initialized by various methods).
There are a number of bool parameters that should really be
EnumName.yes/no.
The only candidate I can spot is the Socket.blocking property. What did I
miss?
(Address.toHostString and toAddressString are private)
The current approach that appears to be required for connecting to a
remote host kind of stinks:
Socket sock = null;
foreach(info, getAddressInfo("www.digitalmars.com")) {
try {
sock = new Socket(info); // will throw if can't create a
socket based on info
sock.connect(info.address);
break;
} catch (Exception e) {
sock = null;
}
}
if (sock is null)
// unable to connect via any available method!
It's a question of how much gruntwork should the network module abstract
away. FWIW, the situation is similar with Python:
http://docs.python.org/library/socket.html (scroll down to second "Echo
client program" example)
I've heard opinions on IRC that std.socket should definitely not conflate
connections with DNS lookups, thus a Socket.connect(string hostname)
method wouldn't belong.
As an aside… From your comments, I gather that you're not terribly happy
with certain design requirements imposed by the existing std.socket.
Why not create an entirely new API in std.socket2 and not worry about
it? Would your design change enough to warrant doing this?
I'm not sure if I can find the time and commitment to design an entirely
new socket API at the moment. Simply put, I tried to improve the existing
module without breaking too much.
--
Best regards,
Vladimir mailto:[email protected]
