Thanks for the review.
On Sun, 05 Sep 2010 15:29:36 +0900, Andrei Alexandrescu
<[email protected]> wrote:
I already wrote the following code review for
http://bitbucket.org/repeatedly/scrap/src/tip/socket.d before seeing
Masahiro's new message that we should scrap that review. I'm sending
this in case it applies to his new work based on Asio.
* Line 67: don't use typedef - it will go away
OK.
* 87: feel free to insert a static assert(sockaddr_storage.sizeof ==
...) to make sure the compiler did as you expected.
I think static assert doesn't need because the detail of sockaddr_storage
is not important.
I will add sockaddr_storage to std.c.windows.
* 270: enforce(val, new SocketException(...))
I forgot to rewrite.
* 304-309: lowercase? We have visibility protection because of
AddressFamily.
OK.
* 362: Why is Protocol a class? It has all public members and no
overridable methods. (Also, constructors and public members often are
questionable.)
* 433: Same question about Service.
* 539: And same question about InternetHost.
* 546: why not a ref hostent?
* 553: no need for the .idup
I don't know the detail. I will remove those classes in new module.
* 663: I'm weary about every module adding its own exception type, but
this is subject to a separate investigation we should do for all of
Phobos.
I agree. I think Phobos guide should describe the exception design.
* 694-698: Public members of a class mean that I can change any of them
to random values without breaking the consistency of the object. Is that
true for AddressInfo?
I think yes for AddressInfo bacause AddressInfo is a container for
getaddrinfo.
I will change AddressInfo to struct.
* 752: ref addrinfo
OK.
* 777 and others: which of these methods do you think should be
overridable? Probably a small subset if any. Then they should be final.
I mentioned above.
* 947: Is there something that naturally differentiates the two types of
addresses? If so, you could get rid of Type.
It's a matter of taste.
In general, User uses AddressInfo for Socket.
So, IPv4Address and IPv6Address are almost invisible.
I think IPAddress only is not handy if User uses IPv4 or IPv6
representation directly.
I will add some functions to IPv4 and IPv6 in new module.
* 1294: I'm not an IPv6 pro, but looks like a little hierarchy would
work here (with e.g. IPv4 and IPv6 inheriting a common base). Maybe
LocalAddress could be snuck in too.
What is a common base?
IP Address contains IPv4 and IPv6, but IPv4 and IPv6 doesn't inherit IP
Address.
In addition, IPv6 doesn't have the compatibility to IPv4.
* 2310: what does byes mean?
I don't know, but I think byes stands for "blocking yes".
* 2520, 2541 etc.: Return size_t.
Oops.
* 2934: select() has well-known issues. Any plans to support the newer
system-dependent APIs or libevent?
I will remove select() from Socket in new module.
I am thinking about std.event.
This module supports system-dependent APIs(kqueue, epoll, etc...) and
abstraction layer.
Masahiro
_______________________________________________
phobos mailing list
[email protected]
http://lists.puremagic.com/mailman/listinfo/phobos
