Hi Dirk, --- Dirk Moermans <dirkmoermans@gmail.com> wrote: <snip>
I like the acceptor-connector pattern as used in ACE. It seems that you do use an acceptor class, but did not implement connector classes. It would be nice to have separate classes for connection-establishment and for connection-handling, for both client- and server-side (separation of concerns).
Early on asio had a separate socket_connector class, but it was removed, mainly because: - It does not correspond to a real operating system resource in BSD-based APIs. - You often need to manipulate a socket before establishing a connection, e.g. binding, setting options. - It prevented an implementation of asynchronous connect on Win32 using ConnectEx (still on my to-do list). By this I mean that if you had a separate connector it could be constructed to take a different demuxer to the socket being connected. However with ConnectEx, the completion event is delivered through the IO completion port (i.e. demuxer) of the socket.
I dislike the use of the "services pattern". It might make sense to put the service classes in the details section but they do not belong in the public interface. Such a design would not be appropriate for standardization. I wonder if you can reach the same goals by using concept-checks and some socket_traits classes instead of duplicating every interface.
Not sure what you mean here exactly. Can you elaborate?
I think the following code-snippet in the tutorial could be written more elegantly, through an interface change of the deadline_timer:
{ boost::asio::deadline_timer t; ... t.expires_at(t.expires_at() + boost::posix_time::seconds(1)); }
by giving direct access to the time member.
t.expiry_date()+=boost::posix_time::seconds(1); For more complex cases or error-checking, some proxy classes could be used.
Doesn't such an approach force a particular internal implementation on the timer? The current interface permits the timer to store the expiry in an efficient native format, if required, and to just convert when the expires_*() functions are accessed.
You might want to add swap member-functions implemented in e.g. the address class.
Fair enough.
You could overload more functions that take a const std::string& parameter with a const char* parameter (e.g. the host constructor).
The std::string constructor from const char* should take care of these use cases, shouldn't it? What additional issue do you see a const char* parameter addressing?
You seem to return a std::string in the some member functions of host and address classes, where you could return a const std::string&. e.g.
in host.hpp std::string host::name() const
In my opinion, returning a const reference unnecessarily couples the class's interface to its implementation. By that I mean returning a const reference would require the host class contain a std::string data member. That may be true of the current implementation, but it may not always be the best option.
-- implementation
Why do you catch exceptions by reference instead of const reference :-) ?
Never really thought about it, it has just been the idiomatic usage in all code I've worked on.
Why do you roll out your own threads-implementation and don't rely on boost.threads . It would improve consistency among boost-libraries?
When developing asio I did not want to require dependency on anything other than the boost header files, whereas boost.threads requires you to link against the library. The asio thread implementation is completely internal though, with no effect on the public interface, so it may use boost.threads in the future without impacting application source code. You're also free, of course, to use boost.threads alongside asio (e.g. to call demuxer::run() in a separate thread).
The basic demuxer-work class is copy-constructable so that it may be used as a data member in a handler class. It is not assignable. I have never seen this idiom in a correctly designed class, so it would be nice to elaborate a bit on it.
It behaves more like a reference, i.e. no default construction or assigning. This is based on the requirements placed by asio on completion handlers, i.e. CopyConstructible, but not required to be DefaultConstructible or Assignable.
Documentation: I would like to see a clear distinction between what is considered a public and private class. A split between basic and advanced topics would ease understanding as well.
Yep, it's on my to-do list to split the documentation in this way.
it would improve readability if you would omit the boost::asio namespace throughout the tutorial. Everybody knows the library-name when reading the tutorial. adding a line "use namespace::boost::asio" in the beginning is sufficient.
No worries. I'm just in the habit of fully qualifying all names. Cheers, Chris