From: Peter Dimov (pdimov_at_[hidden])
Date: 2005-04-22 04:42:47
I like the proposal. It shows a certain polish that only comes with
real-world experience with a design. ;-)
I have the following comments for now, based on a not-very-deep look at the
document and the header:
1. network root class
In my opinion, this class is not needed and should be removed. It
complicates the design, forces a global variable on the user, and is not
library-friendly. For example, if lib1 and lib2 use the network, the user
can't pass an address created by lib1 to lib2. The global state modeled by
the network class should be an implementation detail.
2. address
The address should be a standalone class and not tied to a particular
network. It should be a simple entity and not a hierarchical container.
Logical to physical resolution should take a source logical address and
produce a container of physical addresses.
The address should not be created with an URL, that is, a TCP connection to
www.example.com should be represented by the address
tcp:/www.example.com:80, leaving http://www.example.com reserved for the
stream obtained by the HTTP GET / query, not for the raw protocol stream. A
connection to the serial port should probably be represented by
"com1:/38400,n,8,1", and so on.
I think that the UDP broadcast address should be represented by
udp:/0.0.0.0, and the TCP loopback should be tcp:/127.0.0.1, as usual. But I
may be wrong.
3. Minor stylistic issues
p->get_X() should be p->X(), p->new_Y() should probably be p->create_Y(),
although it might be better to move to net::create_Y( p ) or even to Y( p )
and reference semantics under the hood.
The destructors should be protected.
4. MT-centric model
The general idea of an asynchronous callback dispatcher is sound, but the
threading decision should be left to the user. The library should provide
void net::poll( timeout tm );
that delivers any pending callbacks from the context of the thread that
called poll, and
void net::async_poll( error_callback );
that acts "as if" it executes net::poll( infinite ) repeatedly in one or
more background threads. (error_callback will be called on errors; the
synchronous variant will probably throw an exception instead.)
Whether the library uses multiple threads under the hood, or how many, is
implementation defined, even if async_poll is not called; this depends on
which model delivers the best performance on the specific platform.
5. read_later, etc
I think that read_later should be dropped. async_read is enough, in my
opinion.
The functionality of write_later should be achievable with a call to
async_write with size 0; write_later can be dropped, too.
async_read should not take a buffer; instead, the callback should receive a
pointer to a buffer managed by the library that is guaranteed to be valid
for the duration of the callback. (Not by default, at least.)
async_write (by default) should not assume that the passed buffer stays
valid after async_write returns.
Rationale: buffer management is a pain with multiple callbacks active ;-)
That's it for now. Please don't take these as criticisms and wherever you
see "should" read "in my humble opinion, the design probably could be
enhanced by".
Thanks for reading.