Re: Code review
On Jan 6, 9:58 am, Ebenezer <woodbria...@gmail.com> wrote:
On Jan 6, 8:11 am, Leigh Johnston <le...@i42.co.uk> wrote:
Why do you think anyone would spend time reviewing all your code for
free? Reviewing a small class for free is probably not asking too mu=
ch
but not entire an archive containing multiple non-trivial source files.
The way I see it, some people may not want to review any of it,
some may review a little of it and others may choose to review a
lot of it. I'll take what I can get. People thinking about
using the CMW may be willing to put in some effort to help me
improve it.
I have a specific question that I've come across recently. It has to
with exceptions and control flow. This is from the CMWA.
cmwAmbassador::cmwAmbassador(char const* configfile) :
sendbuf(200000), buf(40000),
localsendbuf(4096),
#ifdef BIG_ENDIANS
byteOrder(most_significant_first),
#else
byteOrder(least_significant_first),
#endif
configData(configfile)
{
fd_set master; // master file descriptor list
fd_set read_fds; // temp file descriptor list for select()
FD_ZERO(&master);
FD_ZERO(&read_fds);
sock_type CMWfd = login();
sock_type listener = serverPrep(configData.portnumber);
// Add the listener to the master set
FD_SET(CMWfd, &master);
FD_SET(listener, &master);
#undef max // for Windows
int32_t fdmax = std::max(listener, CMWfd);
for (;;) {
read_fds = master;
if (select(fdmax + 1, &read_fds, NULL, NULL, NULL) == -1) {
if (EINTR == errno) {
continue;
}
throw failure("select() failed with errno of ") << errno;
}
for (int32_t sock = 0; sock <= fdmax; ++sock) {
if (FD_ISSET(sock, &read_fds)) {
if (sock == CMWfd) {
try {
if (!buf.GotPacket()) {
continue;
}
} catch(eof const& ex) {
closeSocket(CMWfd);
FD_CLR(CMWfd, &master);
flex_string<char> errorMsg = "CMW crashed before your
request was processed.";
transactions_t::iterator itr =
pendingTransactions.begin();
transactions_t::iterator endit =
pendingTransactions.end();
for (; itr != endit; ++itr) {
localsendbuf.sock_ = (*itr).second.sock;
msg_shepherd::Send(localsendbuf, false, errorMsg);
closeSocket((*itr).second.sock);
FD_CLR((*itr).second.sock, &master);
}
pendingTransactions.clear();
CMWfd = login();
fdmax = std::max(listener, CMWfd);
FD_SET(CMWfd, &master);
continue; // This line could go away if I
// move the code below up into the try.
}
int32_t localsock = mediateResponse();
if (localsock != 0) {
closeSocket(localsock);
FD_CLR(localsock, &master);
}
} else {
....
That's the way I have it currently. I've thought about changing it
to the following, picking up from the last for statement in the above:
for (int32_t sock = 0; sock <= fdmax; ++sock) {
if (FD_ISSET(sock, &read_fds)) {
if (sock == CMWfd) {
try {
if (!buf.GotPacket()) {
continue;
}
int32_t localsock = mediateResponse();
if (localsock != 0) {
closeSocket(localsock);
FD_CLR(localsock, &master);
}
} catch(eof const& ex) {
closeSocket(CMWfd);
FD_CLR(CMWfd, &master);
flex_string<char> errorMsg = "CMW crashed before your
request was processed.";
transactions_t::iterator itr =
pendingTransactions.begin();
transactions_t::iterator endit =
pendingTransactions.end();
for (; itr != endit; ++itr) {
localsendbuf.sock_ = (*itr).second.sock;
msg_shepherd::Send(localsendbuf, false, errorMsg);
closeSocket((*itr).second.sock);
FD_CLR((*itr).second.sock, &master);
}
pendingTransactions.clear();
CMWfd = login();
fdmax = std::max(listener, CMWfd);
FD_SET(CMWfd, &master);
}
} else {
....
That way the continue statement in the catch block goes away,
but I don't like having to move the call to mediateResponse from
outside the try to inside of it in this form. That function isn't
supposed to throw eof exceptions. Any advice on this?