Hi, I vote to accept asio into boost. It will still require a lot of work and is by no means finished. My impression is that most issues can be solved incrementally though. The supported features are sufficient (number of platforms + IPV4) to prove that the concepts will work. The documentation is insufficient to understand the interface. I had to read the implementation and the examples. I have spent 8 hours on the review (reading documentation and reading code, I have not written any code using asio). I have experience with ACE, MFC and socket.h (mostly synchronous though, so asio has been an eye-opener for me). This is my first boost-review. -- design: 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). 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. I did read the description in the design section and I am still not convinced that they are a good solution for the stated problems. 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. You might want to add swap member-functions implemented in e.g. the address class. You could overload more functions that take a const std::string& parameter with a const char* parameter (e.g. the host constructor). 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 -- implementation Why do you catch exceptions by reference instead of const reference :-) ? Why do you roll out your own threads-implementation and don't rely on boost.threads . It would improve consistency among boost-libraries? 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. 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. 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. I did not have time to look in detail into the scatter-gather mechanism, nor the demuxer classes, nor the ssl implementation ... so I am only scratching the surface here. I am impressed by the library and I would use it. Dirk
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
Christopher Kohlhoff wrote:
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?
Actually, no. It's pretty easy to write a proxy object to do this stuff. A minimal object, just for the above example: class expiry_date_proxy { deadline_timer &timer; public: expiry_date_proxy(deadline_timer &t) : timer(t) {} expiry_date_proxy & operator +=(timespan ts) { timer.expires_at(timer.expires_at() + ts); } }; class deadline_timer { ... expiry_date_proxy expiry_date() { return expiry_date_proxy(*this); } }; I'm not saying that this necessarily should be done (in particular, where do you stop making properties?), but it's possible. Sebastian Redl
Hi Sebastian, --- Sebastian Redl <sebastian.redl@getdesigned.at> wrote:
Christopher Kohlhoff wrote:
Doesn't such an approach force a particular internal implementation on the timer?
Actually, no. It's pretty easy to write a proxy object to do this stuff. A minimal object, just for the above example:
class expiry_date_proxy { deadline_timer &timer;
public: expiry_date_proxy(deadline_timer &t) : timer(t) {}
expiry_date_proxy & operator +=(timespan ts) { timer.expires_at(timer.expires_at() + ts); } };
class deadline_timer { ... expiry_date_proxy expiry_date() { return expiry_date_proxy(*this); } };
I'm not saying that this necessarily should be done (in particular, where do you stop making properties?), but it's possible.
Oh yeah, fair enough. Such a proxy approach could also be done external to the object, e.g: expiry(timer) += seconds(5); The proxy/property approach doesn't seem idiomatic C++ to me, but that's probably just my background. If resetting the expiry relative to the current expiry is a common use case perhaps it is deserving of its own member function, e.g.: timer.expires_from_current(seconds(5)); However when reworking the interface into its current form I chose a more minimalist approach. Cheers, Chris
| -----Original Message----- | From: boost-bounces@lists.boost.org | [mailto:boost-bounces@lists.boost.org] On Behalf Of Dirk Moermans | Sent: 20 December 2005 21:27 | To: boost@lists.boost.org | Subject: [boost] asio review | | 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. I would make a slightly different recommendation - and it applies to all code which has 'educational' intent (as opposed to writing it quickly and concisely). IMO the clearest way is to LIST the bits that are being demonstrated explictly with using statements (rather than a use namespace), for example if we were trying to introduce iostreams for the first time #include <iostream> using std::cout; using std::cin; using std::endl; will make clear which functions are being used, and indeed include info on where they are from, and reduces the risk of unexpected effects from a 'global' using namespace std; If there is a chance of ambiguity, then placing the using statement close to (first) use may be best, for example: using std::tr1::cbrt; // Convenient to avoid getting global ::cbrt by mistake. cout << cbrt(27.) << endl; If it is just a reminder or aside, of why a file is being included, then then #include <iostream> // for cout, endl... might be more concise. Paul -- Paul A Bristow Prizet Farmhouse, Kendal, Cumbria UK LA8 8AB Phone and SMS text +44 1539 561830, Mobile and SMS text +44 7714 330204 mailto: pbristow@hetp.u-net.com http://www.hetp.u-net.com/index.html http://www.hetp.u-net.com/Paul%20A%20Bristow%20info.html
participants (4)
-
Christopher Kohlhoff -
Dirk Moermans -
Paul A Bristow -
Sebastian Redl